Received-SPF: pass (google.com: domain of linux-kernel+bounces-198137-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240531141152.327592-1-kikuchan98@gmail.com> <20240531141152.327592-6-kikuchan98@gmail.com>
 <851280ad-ac0e-47d1-99e2-4f3b5ea29f2f@kernel.org> <CAG40kxEbMQc-ni0HDVR7rtj48aFu-jz8sYUAO+fdmZSmXWrizw@mail.gmail.com>
 <da382d43-fa82-44c0-9630-086f59e6efa2@kernel.org>
In-Reply-To: <da382d43-fa82-44c0-9630-086f59e6efa2@kernel.org>
From: Hironori KIKUCHI <kikuchan98@gmail.com>
Date: Sun, 2 Jun 2024 15:15:13 +0900
Message-ID: <CAG40kxHKdC=uwyWzsBo1LTAXARDQGs0N4TBdD5nE1zhos48cbg@mail.gmail.com>
Subject: Re: [PATCH 5/5] dt-bindings: pwm: sun20i: Add options to select a
 clock source and DIV_M
To: Krzysztof Kozlowski <krzk@kernel.org>
Cc: linux-kernel@vger.kernel.org, =?UTF-8?Q?Uwe_Kleine=2DK=C3=B6nig?= <ukleinek@kernel.org>, 
	Rob Herring <robh@kernel.org>, Krzysztof Kozlowski <krzk+dt@kernel.org>, Conor Dooley <conor+dt@kernel.org>, 
	Chen-Yu Tsai <wens@csie.org>, Jernej Skrabec <jernej.skrabec@gmail.com>, 
	Samuel Holland <samuel@sholland.org>, Aleksandr Shubin <privatesub2@gmail.com>, 
	Cheo Fusi <fusibrandon13@gmail.com>, linux-pwm@vger.kernel.org, 
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, 
	linux-sunxi@lists.linux.dev
Content-Type: text/plain; charset="UTF-8"

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.