diff mbox series

[5/5] dt-bindings: pwm: sun20i: Add options to select a clock source and DIV_M

Message ID 20240531141152.327592-6-kikuchan98@gmail.com (mailing list archive)
State New, archived
Headers show
Series Add support for Allwinner H616 PWM | expand

Commit Message

Hironori KIKUCHI May 31, 2024, 2:11 p.m. UTC
This patch adds new options to select a clock source and DIV_M register
value for each coupled PWM channels.

Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com>
---
 .../bindings/pwm/allwinner,sun20i-pwm.yaml    | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Comments

Krzysztof Kozlowski May 31, 2024, 2:43 p.m. UTC | #1
On 31/05/2024 16:11, Hironori KIKUCHI wrote:
> This patch adds new options to select a clock source and DIV_M register
> value for each coupled PWM channels.

Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

Bindings are before their users. This should not be last patch, because
this implies there is no user.

This applies to all variants? Or the one you add? Confused...

> 
> Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com>
> ---
>  .../bindings/pwm/allwinner,sun20i-pwm.yaml    | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> index b9b6d7e7c87..436a1d344ab 100644
> --- a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> @@ -45,6 +45,25 @@ properties:
>      description: The number of PWM channels configured for this instance
>      enum: [6, 9]
>  
> +  allwinner,pwm-pair-clock-sources:
> +    description: The clock source names for each PWM pair
> +    items:
> +      enum: [hosc, apb]
> +    minItems: 1
> +    maxItems: 8

Missing type... and add 8 of such items to your example to make it complete.

> +
> +  allwinner,pwm-pair-clock-prescales:
> +    description: The prescale (DIV_M register) values for each PWM pair
> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> +    items:
> +      items:
> +        minimum: 0
> +        maximum: 8
> +      minItems: 1
> +      maxItems: 1
> +    minItems: 1
> +    maxItems: 8

This does not look like matrix but array.

Why clock DIV cannot be deduced from typical PWM attributes + clock
frequency?

Best regards,
Krzysztof
Hironori KIKUCHI May 31, 2024, 5:57 p.m. UTC | #2
Hello,

> > This patch adds new options to select a clock source and DIV_M register
> > value for each coupled PWM channels.
>
> Please do not use "This commit/patch/change", but imperative mood. See
> longer explanation here:
> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>
> Bindings are before their users. This should not be last patch, because
> this implies there is no user.

I'm sorry, I'll fix them.

> This applies to all variants? Or the one you add? Confused...

Apologies for confusing you. This applies to all variants.

>
> >
> > Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com>
> > ---
> >  .../bindings/pwm/allwinner,sun20i-pwm.yaml    | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> > index b9b6d7e7c87..436a1d344ab 100644
> > --- a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> > +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> > @@ -45,6 +45,25 @@ properties:
> >      description: The number of PWM channels configured for this instance
> >      enum: [6, 9]
> >
> > +  allwinner,pwm-pair-clock-sources:
> > +    description: The clock source names for each PWM pair
> > +    items:
> > +      enum: [hosc, apb]
> > +    minItems: 1
> > +    maxItems: 8
>
> Missing type... and add 8 of such items to your example to make it complete.

Thank you. I'll fix it.

>
> > +
> > +  allwinner,pwm-pair-clock-prescales:
> > +    description: The prescale (DIV_M register) values for each PWM pair
> > +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > +    items:
> > +      items:
> > +        minimum: 0
> > +        maximum: 8
> > +      minItems: 1
> > +      maxItems: 1
> > +    minItems: 1
> > +    maxItems: 8
>
> This does not look like matrix but array.

I wanted to specify values like this:

    allwinner,pwm-pair-clock-prescales = <0>, <1>, <3>;
    allwinner,pwm-pair-clock-sources = "hosc", "apb", "hosc":

These should correspond to each PWM pair.
This way, I thought we might be able to visually understand the relationship
between prescalers and sources, like clock-names and clocks.

Is this notation uncommon, perhaps?

>
> Why clock DIV cannot be deduced from typical PWM attributes + clock
> frequency?

This SoC's PWM system has one shared prescaler and clock source for each pair
of PWM channels. I should have noted this earlier, sorry.

Actually, the original v9 patch automatically deduced the DIV value
from the frequency.
However, because the two channels share a single prescaler, once one channel is
enabled, it affects and restricts the DIV value for the other channel
in the pair.
This introduces a problem of determining which channel should set the shared DIV
value. The original behavior was that the first channel enabled would win.

Instead, this patch try to resolve the issue by specifying these
values for each PWM
pairs deterministically.
That's why it requires the new options.

>
> Best regards,
> Krzysztof

Best regards,
kikuchan.
Krzysztof Kozlowski June 1, 2024, 3:17 p.m. UTC | #3
On 31/05/2024 19:57, Hironori KIKUCHI wrote:
> Hello,
> 
>>> This patch adds new options to select a clock source and DIV_M register
>>> value for each coupled PWM channels.
>>
>> Please do not use "This commit/patch/change", but imperative mood. See
>> longer explanation here:
>> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>>
>> Bindings are before their users. This should not be last patch, because
>> this implies there is no user.
> 
> I'm sorry, I'll fix them.
> 
>> This applies to all variants? Or the one you add? Confused...
> 
> Apologies for confusing you. This applies to all variants.
> 
>>
>>>
>>> Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com>
>>> ---
>>>  .../bindings/pwm/allwinner,sun20i-pwm.yaml    | 19 +++++++++++++++++++
>>>  1 file changed, 19 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
>>> index b9b6d7e7c87..436a1d344ab 100644
>>> --- a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
>>> +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
>>> @@ -45,6 +45,25 @@ properties:
>>>      description: The number of PWM channels configured for this instance
>>>      enum: [6, 9]
>>>
>>> +  allwinner,pwm-pair-clock-sources:
>>> +    description: The clock source names for each PWM pair
>>> +    items:
>>> +      enum: [hosc, apb]
>>> +    minItems: 1
>>> +    maxItems: 8
>>
>> Missing type... and add 8 of such items to your example to make it complete.
> 
> Thank you. I'll fix it.
> 
>>
>>> +
>>> +  allwinner,pwm-pair-clock-prescales:
>>> +    description: The prescale (DIV_M register) values for each PWM pair
>>> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
>>> +    items:
>>> +      items:
>>> +        minimum: 0
>>> +        maximum: 8
>>> +      minItems: 1
>>> +      maxItems: 1
>>> +    minItems: 1
>>> +    maxItems: 8
>>
>> This does not look like matrix but array.
> 
> I wanted to specify values like this:
> 
>     allwinner,pwm-pair-clock-prescales = <0>, <1>, <3>;
>     allwinner,pwm-pair-clock-sources = "hosc", "apb", "hosc":
> 
> These should correspond to each PWM pair.
> This way, I thought we might be able to visually understand the relationship
> between prescalers and sources, like clock-names and clocks.
> 
> Is this notation uncommon, perhaps?

It's still an array.

> 
>>
>> Why clock DIV cannot be deduced from typical PWM attributes + clock
>> frequency?
> 
> This SoC's PWM system has one shared prescaler and clock source for each pair
> of PWM channels. I should have noted this earlier, sorry.
> 
> Actually, the original v9 patch automatically deduced the DIV value
> from the frequency.
> However, because the two channels share a single prescaler, once one channel is
> enabled, it affects and restricts the DIV value for the other channel
> in the pair.
> This introduces a problem of determining which channel should set the shared DIV
> value. The original behavior was that the first channel enabled would win.

There's nothing bad in this.

> 
> Instead, this patch try to resolve the issue by specifying these
> values for each PWM
> pairs deterministically.
> That's why it requires the new options.

This does not solve that wrong divider can be programmed for second
channel in each pair.

Best regards,
Krzysztof
Hironori KIKUCHI June 2, 2024, 6:15 a.m. UTC | #4
Hi Krzysztof,

> On 31/05/2024 19:57, Hironori KIKUCHI wrote:
> > Hello,
> >
> >>> This patch adds new options to select a clock source and DIV_M register
> >>> value for each coupled PWM channels.
> >>
> >> Please do not use "This commit/patch/change", but imperative mood. See
> >> longer explanation here:
> >> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
> >>
> >> Bindings are before their users. This should not be last patch, because
> >> this implies there is no user.
> >
> > I'm sorry, I'll fix them.
> >
> >> This applies to all variants? Or the one you add? Confused...
> >
> > Apologies for confusing you. This applies to all variants.
> >
> >>
> >>>
> >>> Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com>
> >>> ---
> >>>  .../bindings/pwm/allwinner,sun20i-pwm.yaml    | 19 +++++++++++++++++++
> >>>  1 file changed, 19 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> >>> index b9b6d7e7c87..436a1d344ab 100644
> >>> --- a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> >>> +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> >>> @@ -45,6 +45,25 @@ properties:
> >>>      description: The number of PWM channels configured for this instance
> >>>      enum: [6, 9]
> >>>
> >>> +  allwinner,pwm-pair-clock-sources:
> >>> +    description: The clock source names for each PWM pair
> >>> +    items:
> >>> +      enum: [hosc, apb]
> >>> +    minItems: 1
> >>> +    maxItems: 8
> >>
> >> Missing type... and add 8 of such items to your example to make it complete.
> >
> > Thank you. I'll fix it.
> >
> >>
> >>> +
> >>> +  allwinner,pwm-pair-clock-prescales:
> >>> +    description: The prescale (DIV_M register) values for each PWM pair
> >>> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> >>> +    items:
> >>> +      items:
> >>> +        minimum: 0
> >>> +        maximum: 8
> >>> +      minItems: 1
> >>> +      maxItems: 1
> >>> +    minItems: 1
> >>> +    maxItems: 8
> >>
> >> This does not look like matrix but array.
> >
> > I wanted to specify values like this:
> >
> >     allwinner,pwm-pair-clock-prescales = <0>, <1>, <3>;
> >     allwinner,pwm-pair-clock-sources = "hosc", "apb", "hosc":
> >
> > These should correspond to each PWM pair.
> > This way, I thought we might be able to visually understand the relationship
> > between prescalers and sources, like clock-names and clocks.
> >
> > Is this notation uncommon, perhaps?
>
> It's still an array.

Oh I understood and clear. Thank you.

> >> Why clock DIV cannot be deduced from typical PWM attributes + clock
> >> frequency?
> >
> > This SoC's PWM system has one shared prescaler and clock source for each pair
> > of PWM channels. I should have noted this earlier, sorry.
> >
> > Actually, the original v9 patch automatically deduced the DIV value
> > from the frequency.
> > However, because the two channels share a single prescaler, once one channel is
> > enabled, it affects and restricts the DIV value for the other channel
> > in the pair.
> > This introduces a problem of determining which channel should set the shared DIV
> > value. The original behavior was that the first channel enabled would win.
>
> There's nothing bad in this.
>
> >
> > Instead, this patch try to resolve the issue by specifying these
> > values for each PWM
> > pairs deterministically.
> > That's why it requires the new options.
>
> This does not solve that wrong divider can be programmed for second
> channel in each pair.
>

Let me illustrate the connection of a paired PWM channels to be sure.

.    +------+                   +--------------+  +------+
.    + HOSC +--+             +--+ prescale_k 0 +--+ PWM0 |
.    +------+  |  +-------+  |  +--------------+  +------+
.              +--+ DIV_M +--+
.    +------+  |  +-------+  |  +--------------+  +------+
.    + APBx +--+             +--+ prescale_k 1 +--+ PWM1 |
.    +------+                   +--------------+  +------+
.          CLK_SRC

The PWM0 and PWM1 share DIV_M and CLK_SRC for them, and (not
illustrated) PWM2 and PWM3 share another DIV_M and another CLK_SRC for
them, and so on.
The DIV_M ranges from 0 to 8 and is used as a 1 / 2^DIV_M prescaler,
prescale_k ranges from 0 to 255 and is used as a 1 / (prescale_k + 1)
prescaler.

In the original v9 patch, enabling PWM0 determines CLK_SRC and
calculates DIV_M from the period that is going to be set.
Once the CLK_SRC and DIV_M are fixed, they cannot be changed until
both channels are disabled, unless PWM0 is the only enabled channel.

Looks good so far, but there is a pitfall.

Selecting CLK_SRC and DIV_M means it defines the PWM resolution of the
period and duty cycle for the pair of the PWM channels.
In other words, the resolution is determined by the (most likely the
very first) period, which can be arbitrary.

Consider an application that uses PWM channels to generate a square
wave in stereo.
The very first musical note played defines the entire resolution for
the subsequent notes.
The music quality depends on the first note.

The problem is, there is NO way to fixate the resolution to be used.

The proposed method provides a simple way to deterministically fixate
the resolution.
(ofcourse, prescale_k is still calculated by period to be set)

> Best regards,
> Krzysztof

Best regards,
kikuchan.
Andre Przywara June 3, 2024, 12:09 a.m. UTC | #5
On Sun, 2 Jun 2024 15:15:13 +0900
Hironori KIKUCHI <kikuchan98@gmail.com> wrote:

Hi Kikuchan,

> Hi Krzysztof,
> 
> > On 31/05/2024 19:57, Hironori KIKUCHI wrote:  
> > > Hello,
> > >  
> > >>> This patch adds new options to select a clock source and DIV_M register
> > >>> value for each coupled PWM channels.  
> > >>
> > >> Please do not use "This commit/patch/change", but imperative mood. See
> > >> longer explanation here:
> > >> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
> > >>
> > >> Bindings are before their users. This should not be last patch, because
> > >> this implies there is no user.  
> > >
> > > I'm sorry, I'll fix them.
> > >  
> > >> This applies to all variants? Or the one you add? Confused...  
> > >
> > > Apologies for confusing you. This applies to all variants.
> > >  
> > >>  
> > >>>
> > >>> Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com>
> > >>> ---
> > >>>  .../bindings/pwm/allwinner,sun20i-pwm.yaml    | 19 +++++++++++++++++++
> > >>>  1 file changed, 19 insertions(+)
> > >>>
> > >>> diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> > >>> index b9b6d7e7c87..436a1d344ab 100644
> > >>> --- a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> > >>> +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> > >>> @@ -45,6 +45,25 @@ properties:
> > >>>      description: The number of PWM channels configured for this instance
> > >>>      enum: [6, 9]
> > >>>
> > >>> +  allwinner,pwm-pair-clock-sources:
> > >>> +    description: The clock source names for each PWM pair
> > >>> +    items:
> > >>> +      enum: [hosc, apb]
> > >>> +    minItems: 1
> > >>> +    maxItems: 8  
> > >>
> > >> Missing type... and add 8 of such items to your example to make it complete.  
> > >
> > > Thank you. I'll fix it.
> > >  
> > >>  
> > >>> +
> > >>> +  allwinner,pwm-pair-clock-prescales:
> > >>> +    description: The prescale (DIV_M register) values for each PWM pair
> > >>> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > >>> +    items:
> > >>> +      items:
> > >>> +        minimum: 0
> > >>> +        maximum: 8
> > >>> +      minItems: 1
> > >>> +      maxItems: 1
> > >>> +    minItems: 1
> > >>> +    maxItems: 8  
> > >>
> > >> This does not look like matrix but array.  
> > >
> > > I wanted to specify values like this:
> > >
> > >     allwinner,pwm-pair-clock-prescales = <0>, <1>, <3>;
> > >     allwinner,pwm-pair-clock-sources = "hosc", "apb", "hosc":
> > >
> > > These should correspond to each PWM pair.
> > > This way, I thought we might be able to visually understand the relationship
> > > between prescalers and sources, like clock-names and clocks.
> > >
> > > Is this notation uncommon, perhaps?  
> >
> > It's still an array.  
> 
> Oh I understood and clear. Thank you.
> 
> > >> Why clock DIV cannot be deduced from typical PWM attributes + clock
> > >> frequency?  
> > >
> > > This SoC's PWM system has one shared prescaler and clock source for each pair
> > > of PWM channels. I should have noted this earlier, sorry.
> > >
> > > Actually, the original v9 patch automatically deduced the DIV value
> > > from the frequency.
> > > However, because the two channels share a single prescaler, once one channel is
> > > enabled, it affects and restricts the DIV value for the other channel
> > > in the pair.
> > > This introduces a problem of determining which channel should set the shared DIV
> > > value. The original behavior was that the first channel enabled would win.  
> >
> > There's nothing bad in this.
> >  
> > >
> > > Instead, this patch try to resolve the issue by specifying these
> > > values for each PWM
> > > pairs deterministically.
> > > That's why it requires the new options.  
> >
> > This does not solve that wrong divider can be programmed for second
> > channel in each pair.
> >  
> 
> Let me illustrate the connection of a paired PWM channels to be sure.
> 
> .    +------+                   +--------------+  +------+
> .    + HOSC +--+             +--+ prescale_k 0 +--+ PWM0 |
> .    +------+  |  +-------+  |  +--------------+  +------+
> .              +--+ DIV_M +--+
> .    +------+  |  +-------+  |  +--------------+  +------+
> .    + APBx +--+             +--+ prescale_k 1 +--+ PWM1 |
> .    +------+                   +--------------+  +------+
> .          CLK_SRC
> 
> The PWM0 and PWM1 share DIV_M and CLK_SRC for them, and (not
> illustrated) PWM2 and PWM3 share another DIV_M and another CLK_SRC for
> them, and so on.
> The DIV_M ranges from 0 to 8 and is used as a 1 / 2^DIV_M prescaler,
> prescale_k ranges from 0 to 255 and is used as a 1 / (prescale_k + 1)
> prescaler.
> 
> In the original v9 patch, enabling PWM0 determines CLK_SRC and
> calculates DIV_M from the period that is going to be set.
> Once the CLK_SRC and DIV_M are fixed, they cannot be changed until
> both channels are disabled, unless PWM0 is the only enabled channel.
> 
> Looks good so far, but there is a pitfall.
> 
> Selecting CLK_SRC and DIV_M means it defines the PWM resolution of the
> period and duty cycle for the pair of the PWM channels.
> In other words, the resolution is determined by the (most likely the
> very first) period, which can be arbitrary.

So I understand the problem, but I don't think expressing this in the
devicetree is the right solution. It seems like a tempting pragmatical
approach, but it sounds like the wrong way: this is not a hardware
*description* of any kind, but rather a way to describe a certain user
intention or a configuration. So this looks like a rather embedded
approach to me, where you have a certain fixed register setup in mind,
and want to somehow force this to the hardware.
Another problem with this approach is that it doesn't really cover the
sysfs interface, which is very dynamic by nature.

I have some questions / ideas, and would love to hear some feedback on
them:
- If some PWM channels are "linked", I don't think there is much we can
  do about it: it's a hardware limitation. The details of that is
  already "encoded" in the compatible string, I'd say, so there is no
  need for further description in the devicetree. Any PWM user on those
  boards would probably need to know about the shortcomings, and either
  use different channels for wildly different PWM setups, or accept
  that there are actually only three freely programmable PWM channels.
- Does the PWM subsystem already have a way to model linked channels?
  Maybe that problem is solved already elsewhere?
- Previous Allwinner PWM IP was restricted to use the 24 MHz
  oscillator only, and people seem to have survived with that. Can we
  not just restrict ourselves to one clock source for those linked
  channels? I would assume that the PWM frequency is less important
  than the duty cycle? 
- Can't we just return an error if some conflicting setup requests are
  made? At the expense of this seeming somewhat random to the user,
  because it depends on the order of requests? But people could then
  react on the returned error value?

In general, I wonder what the real use cases are, maybe it's not a
problem in real life? Do you have a concrete issue at hand, or is this
just thinking about all potential use cases - which is honourable, but
maybe a bit over the top here?

Cheers,
Andre

> Consider an application that uses PWM channels to generate a square
> wave in stereo.
> The very first musical note played defines the entire resolution for
> the subsequent notes.
> The music quality depends on the first note.
> 
> The problem is, there is NO way to fixate the resolution to be used.
> 
> The proposed method provides a simple way to deterministically fixate
> the resolution.
> (ofcourse, prescale_k is still calculated by period to be set)
> 
> > Best regards,
> > Krzysztof  
> 
> Best regards,
> kikuchan.
>
Hironori KIKUCHI June 3, 2024, 8:42 a.m. UTC | #6
Hi Andre,

Thank you for your reply.

2024年6月3日(月) 9:10 Andre Przywara <andre.przywara@arm.com>:
>
> On Sun, 2 Jun 2024 15:15:13 +0900
> Hironori KIKUCHI <kikuchan98@gmail.com> wrote:
>
> Hi Kikuchan,
>
> > Hi Krzysztof,
> >
> > > On 31/05/2024 19:57, Hironori KIKUCHI wrote:
> > > > Hello,
> > > >
> > > >>> This patch adds new options to select a clock source and DIV_M register
> > > >>> value for each coupled PWM channels.
> > > >>
> > > >> Please do not use "This commit/patch/change", but imperative mood. See
> > > >> longer explanation here:
> > > >> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
> > > >>
> > > >> Bindings are before their users. This should not be last patch, because
> > > >> this implies there is no user.
> > > >
> > > > I'm sorry, I'll fix them.
> > > >
> > > >> This applies to all variants? Or the one you add? Confused...
> > > >
> > > > Apologies for confusing you. This applies to all variants.
> > > >
> > > >>
> > > >>>
> > > >>> Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com>
> > > >>> ---
> > > >>>  .../bindings/pwm/allwinner,sun20i-pwm.yaml    | 19 +++++++++++++++++++
> > > >>>  1 file changed, 19 insertions(+)
> > > >>>
> > > >>> diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> > > >>> index b9b6d7e7c87..436a1d344ab 100644
> > > >>> --- a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> > > >>> +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> > > >>> @@ -45,6 +45,25 @@ properties:
> > > >>>      description: The number of PWM channels configured for this instance
> > > >>>      enum: [6, 9]
> > > >>>
> > > >>> +  allwinner,pwm-pair-clock-sources:
> > > >>> +    description: The clock source names for each PWM pair
> > > >>> +    items:
> > > >>> +      enum: [hosc, apb]
> > > >>> +    minItems: 1
> > > >>> +    maxItems: 8
> > > >>
> > > >> Missing type... and add 8 of such items to your example to make it complete.
> > > >
> > > > Thank you. I'll fix it.
> > > >
> > > >>
> > > >>> +
> > > >>> +  allwinner,pwm-pair-clock-prescales:
> > > >>> +    description: The prescale (DIV_M register) values for each PWM pair
> > > >>> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > > >>> +    items:
> > > >>> +      items:
> > > >>> +        minimum: 0
> > > >>> +        maximum: 8
> > > >>> +      minItems: 1
> > > >>> +      maxItems: 1
> > > >>> +    minItems: 1
> > > >>> +    maxItems: 8
> > > >>
> > > >> This does not look like matrix but array.
> > > >
> > > > I wanted to specify values like this:
> > > >
> > > >     allwinner,pwm-pair-clock-prescales = <0>, <1>, <3>;
> > > >     allwinner,pwm-pair-clock-sources = "hosc", "apb", "hosc":
> > > >
> > > > These should correspond to each PWM pair.
> > > > This way, I thought we might be able to visually understand the relationship
> > > > between prescalers and sources, like clock-names and clocks.
> > > >
> > > > Is this notation uncommon, perhaps?
> > >
> > > It's still an array.
> >
> > Oh I understood and clear. Thank you.
> >
> > > >> Why clock DIV cannot be deduced from typical PWM attributes + clock
> > > >> frequency?
> > > >
> > > > This SoC's PWM system has one shared prescaler and clock source for each pair
> > > > of PWM channels. I should have noted this earlier, sorry.
> > > >
> > > > Actually, the original v9 patch automatically deduced the DIV value
> > > > from the frequency.
> > > > However, because the two channels share a single prescaler, once one channel is
> > > > enabled, it affects and restricts the DIV value for the other channel
> > > > in the pair.
> > > > This introduces a problem of determining which channel should set the shared DIV
> > > > value. The original behavior was that the first channel enabled would win.
> > >
> > > There's nothing bad in this.
> > >
> > > >
> > > > Instead, this patch try to resolve the issue by specifying these
> > > > values for each PWM
> > > > pairs deterministically.
> > > > That's why it requires the new options.
> > >
> > > This does not solve that wrong divider can be programmed for second
> > > channel in each pair.
> > >
> >
> > Let me illustrate the connection of a paired PWM channels to be sure.
> >
> > .    +------+                   +--------------+  +------+
> > .    + HOSC +--+             +--+ prescale_k 0 +--+ PWM0 |
> > .    +------+  |  +-------+  |  +--------------+  +------+
> > .              +--+ DIV_M +--+
> > .    +------+  |  +-------+  |  +--------------+  +------+
> > .    + APBx +--+             +--+ prescale_k 1 +--+ PWM1 |
> > .    +------+                   +--------------+  +------+
> > .          CLK_SRC
> >
> > The PWM0 and PWM1 share DIV_M and CLK_SRC for them, and (not
> > illustrated) PWM2 and PWM3 share another DIV_M and another CLK_SRC for
> > them, and so on.
> > The DIV_M ranges from 0 to 8 and is used as a 1 / 2^DIV_M prescaler,
> > prescale_k ranges from 0 to 255 and is used as a 1 / (prescale_k + 1)
> > prescaler.
> >
> > In the original v9 patch, enabling PWM0 determines CLK_SRC and
> > calculates DIV_M from the period that is going to be set.
> > Once the CLK_SRC and DIV_M are fixed, they cannot be changed until
> > both channels are disabled, unless PWM0 is the only enabled channel.
> >
> > Looks good so far, but there is a pitfall.
> >
> > Selecting CLK_SRC and DIV_M means it defines the PWM resolution of the
> > period and duty cycle for the pair of the PWM channels.
> > In other words, the resolution is determined by the (most likely the
> > very first) period, which can be arbitrary.
>
> So I understand the problem, but I don't think expressing this in the
> devicetree is the right solution. It seems like a tempting pragmatical
> approach, but it sounds like the wrong way: this is not a hardware
> *description* of any kind, but rather a way to describe a certain user
> intention or a configuration. So this looks like a rather embedded
> approach to me, where you have a certain fixed register setup in mind,
> and want to somehow force this to the hardware.
> Another problem with this approach is that it doesn't really cover the
> sysfs interface, which is very dynamic by nature.

... Indeed. You're right.
Now I've realized it was a bad idea.
It should be done in sysfs or sysctl perhaps.

>
> I have some questions / ideas, and would love to hear some feedback on
> them:
> - If some PWM channels are "linked", I don't think there is much we can
>   do about it: it's a hardware limitation. The details of that is
>   already "encoded" in the compatible string, I'd say, so there is no
>   need for further description in the devicetree. Any PWM user on those
>   boards would probably need to know about the shortcomings, and either
>   use different channels for wildly different PWM setups, or accept
>   that there are actually only three freely programmable PWM channels.
> - Does the PWM subsystem already have a way to model linked channels?
>   Maybe that problem is solved already elsewhere?
> - Previous Allwinner PWM IP was restricted to use the 24 MHz
>   oscillator only, and people seem to have survived with that. Can we
>   not just restrict ourselves to one clock source for those linked
>   channels? I would assume that the PWM frequency is less important
>   than the duty cycle?
> - Can't we just return an error if some conflicting setup requests are
>   made? At the expense of this seeming somewhat random to the user,
>   because it depends on the order of requests? But people could then
>   react on the returned error value?
>
> In general, I wonder what the real use cases are, maybe it's not a
> problem in real life? Do you have a concrete issue at hand, or is this
> just thinking about all potential use cases - which is honourable, but
> maybe a bit over the top here?

IMHO, it is sufficient to use fixed CLK_SRC and DIV_M values for this
driver, since the default values (CLK_SRC == hosc and DIV_M == 0)
already provide enough range in real life.

What I really care about is minimizing complexity and avoiding surprises.
Although the original method enables an incredibly wide range of the
period, it introduces unpredictability in resolution and inequity in a
pair due to a race in the order of enabling, as a drawback.

If the primary concern is achieving such a wide range, then I think
the original method is the most suitable option.

>
> Cheers,
> Andre
>

Best regards,
kikuchan.
Uwe Kleine-König July 13, 2024, 12:58 p.m. UTC | #7
Hello,

On Mon, Jun 03, 2024 at 05:42:08PM +0900, Hironori KIKUCHI wrote:
> 2024年6月3日(月) 9:10 Andre Przywara <andre.przywara@arm.com>:
> >
> > On Sun, 2 Jun 2024 15:15:13 +0900
> > Hironori KIKUCHI <kikuchan98@gmail.com> wrote:
> >
> > > > On 31/05/2024 19:57, Hironori KIKUCHI wrote:
> > > > >>> ---
> > > > >>>  .../bindings/pwm/allwinner,sun20i-pwm.yaml    | 19 +++++++++++++++++++
> > > > >>>  1 file changed, 19 insertions(+)
> > > > >>>
> > > > >>> diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> > > > >>> index b9b6d7e7c87..436a1d344ab 100644
> > > > >>> --- a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> > > > >>> +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> > > > >>> @@ -45,6 +45,25 @@ properties:
> > > > >>>      description: The number of PWM channels configured for this instance
> > > > >>>      enum: [6, 9]
> > > > >>>
> > > > >>> +  allwinner,pwm-pair-clock-sources:
> > > > >>> +    description: The clock source names for each PWM pair
> > > > >>> +    items:
> > > > >>> +      enum: [hosc, apb]
> > > > >>> +    minItems: 1
> > > > >>> +    maxItems: 8
> > > > >>
> > > > >> Missing type... and add 8 of such items to your example to make it complete.
> > > > >
> > > > > Thank you. I'll fix it.
> > > > >
> > > > >>
> > > > >>> +
> > > > >>> +  allwinner,pwm-pair-clock-prescales:
> > > > >>> +    description: The prescale (DIV_M register) values for each PWM pair
> > > > >>> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > > > >>> +    items:
> > > > >>> +      items:
> > > > >>> +        minimum: 0
> > > > >>> +        maximum: 8
> > > > >>> +      minItems: 1
> > > > >>> +      maxItems: 1
> > > > >>> +    minItems: 1
> > > > >>> +    maxItems: 8
> > > > >>
> > > > >> This does not look like matrix but array.
> > > > >
> > > > > I wanted to specify values like this:
> > > > >
> > > > >     allwinner,pwm-pair-clock-prescales = <0>, <1>, <3>;
> > > > >     allwinner,pwm-pair-clock-sources = "hosc", "apb", "hosc":
> > > > >
> > > > > These should correspond to each PWM pair.
> > > > > This way, I thought we might be able to visually understand the relationship
> > > > > between prescalers and sources, like clock-names and clocks.
> > > > >
> > > > > Is this notation uncommon, perhaps?
> > > >
> > > > It's still an array.
> > >
> > > Oh I understood and clear. Thank you.

For clocks there is already a binding to assign a working configuration.
assigned-clocks, assigned-clock-rates and assigned-clock-parents are the
relevant properties. If you create a clk from the parent clock selector
and mdiv, you can stick to the existing bindings.

> > > [...]
> >
> > So I understand the problem, but I don't think expressing this in the
> > devicetree is the right solution. It seems like a tempting pragmatical
> > approach, but it sounds like the wrong way: this is not a hardware
> > *description* of any kind, but rather a way to describe a certain user
> > intention or a configuration. So this looks like a rather embedded
> > approach to me, where you have a certain fixed register setup in mind,
> > and want to somehow force this to the hardware.
> > Another problem with this approach is that it doesn't really cover the
> > sysfs interface, which is very dynamic by nature.
> 
> ... Indeed. You're right.
> Now I've realized it was a bad idea.
> It should be done in sysfs or sysctl perhaps.

I don't think a driver specific knob somewhere is a practical solution.

Best regards
Uwe
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
index b9b6d7e7c87..436a1d344ab 100644
--- a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
+++ b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
@@ -45,6 +45,25 @@  properties:
     description: The number of PWM channels configured for this instance
     enum: [6, 9]
 
+  allwinner,pwm-pair-clock-sources:
+    description: The clock source names for each PWM pair
+    items:
+      enum: [hosc, apb]
+    minItems: 1
+    maxItems: 8
+
+  allwinner,pwm-pair-clock-prescales:
+    description: The prescale (DIV_M register) values for each PWM pair
+    $ref: /schemas/types.yaml#/definitions/uint32-matrix
+    items:
+      items:
+        minimum: 0
+        maximum: 8
+      minItems: 1
+      maxItems: 1
+    minItems: 1
+    maxItems: 8
+
 allOf:
   - $ref: pwm.yaml#