Re: [PATCH v9 0/3] Add support for Allwinner PWM on D1/T113s/R329 SoCs

Re: [PATCH v9 0/3] Add support for Allwinner PWM on D1/T113s/R329 SoCs

[きくちゃんさん]

Hello Aleksandr,

I had coincidentally developed a PWM driver for the device.
Based on my experience, I find that dynamically changing the coupled
DIV_M value is quite complex.
The current approach has limitations, especially with resolution
changes, which can be unpredictable for users. For example:

  1. Enabling channel A automatically selects DIV_M.
  2. Enabling coupled channel B with a specific period may result in
poor resolution for channel B, as the DIV_M value depends on the
period of channel A.
  3. If channel B is enabled first, channel A may not be enabled if
its period doesn't fit the DIV_M range selected by channel B.

Additionally, using APB as a clock source for the channels would
further complicate the process.

To simplify this, I suggest (maybe for the future) specifying these
values directly in the Device Tree like this:
```
allwinner,pwm-coupled-channel-clock-sources="hosc", "apb", "hosc";
allwinner,pwm-coupled-channel-clock-prescales=<0>, <3>, <8>;
```
This would delegate the complexity to the DT, making the resolution
predictable for users.
As a bonus, it introduces a way to select clock sources for each
coupled channels.

For the meantime, I think it is enough to use fixed "hosc" and <0> for
regular use.

Looking forward to your thoughts.

Best regards,
kikuchan.

Re: [PATCH v9 0/3] Add support for Allwinner PWM on D1/T113s/R329 SoCs

[John Watts]

On Thu, May 23, 2024 at 11:26:07AM +0900, きくちゃんさん wrote:
> Hello Aleksandr,
> 
> I had coincidentally developed a PWM driver for the device.
> Based on my experience, I find that dynamically changing the coupled
> DIV_M value is quite complex.
> The current approach has limitations, especially with resolution
> changes, which can be unpredictable for users. For example:
> 
>   1. Enabling channel A automatically selects DIV_M.
>   2. Enabling coupled channel B with a specific period may result in
> poor resolution for channel B, as the DIV_M value depends on the
> period of channel A.
>   3. If channel B is enabled first, channel A may not be enabled if
> its period doesn't fit the DIV_M range selected by channel B.
> 
> Additionally, using APB as a clock source for the channels would
> further complicate the process.
> 
> To simplify this, I suggest (maybe for the future) specifying these
> values directly in the Device Tree like this:
> ```
> allwinner,pwm-coupled-channel-clock-sources="hosc", "apb", "hosc";
> allwinner,pwm-coupled-channel-clock-prescales=<0>, <3>, <8>;
> ```
> This would delegate the complexity to the DT, making the resolution
> predictable for users.
> As a bonus, it introduces a way to select clock sources for each
> coupled channels.
> 
> For the meantime, I think it is enough to use fixed "hosc" and <0> for
> regular use.
> 
> Looking forward to your thoughts.
> 
> Best regards,
> kikuchan.

I have a somewhat opposite opinion. I've developed a driver too and posted it
on the u-boot mailing list that is deterministic and handles both channels:

https://lore.kernel.org/all/20240518-pwm_d1-v1-0-311fc5fe2248@jookia.org/

It does this by remembering the settings for channels and disabling then
setting both channels at once whenever there's an update.

I think this is a decent enough solution to the problem and just works
automatically without people having to micromanage the controller.

John.

Re: [PATCH v9 0/3] Add support for Allwinner PWM on D1/T113s/R329 SoCs

[きくちゃんさん]

Hello John,

Your method is quite impressive and challenging.

I think PWM is not only for LCD backlighting, but also for signal generation.
For instance, imagine an application software that sends a stream to
one PWM channel to synthesize sound by changing its period (plays a
square wave).
By your method, if the other PWM channel is used for LCD backlighting,
it may flicker repeatedly on DIV_M changes.
(Or simply both channels for streo sound synthesize, you may hear lots
of pop noise)

It means the setting of one channel can affect the other, which users
may not anticipate.

Best regards,
kikuchan.

Re: [PATCH v9 0/3] Add support for Allwinner PWM on D1/T113s/R329 SoCs

[John Watts]

On Thu, May 23, 2024 at 02:30:32PM +0900, きくちゃんさん wrote:
> Hello John,

Hello,

> 
> Your method is quite impressive and challenging.
> 
> I think PWM is not only for LCD backlighting, but also for signal generation.
> For instance, imagine an application software that sends a stream to
> one PWM channel to synthesize sound by changing its period (plays a
> square wave).
> By your method, if the other PWM channel is used for LCD backlighting,
> it may flicker repeatedly on DIV_M changes.
> (Or simply both channels for streo sound synthesize, you may hear lots
> of pop noise)
> 
> It means the setting of one channel can affect the other, which users
> may not anticipate.

Does Linux guarantee a flicker-free experience with setting up PWM
channels, or that it doesn't affect other channels?
How do other drivers handle this situation?

Maybe out-sourcing the clocking to the device tree and letting people set the
clock and common divisor in the DT would be a good idea, with the HOSC and a
common divisor of 1 by default, then error if the value can't be found.

This would work for periods from 41ns up to 687ms by default. Using APB would
give a better resolution as by default it hangs around 5ns which is nice and
divisible by 10.

> 
> Best regards,
> kikuchan.

John.

Re: [PATCH v9 0/3] Add support for Allwinner PWM on D1/T113s/R329 SoCs

[きくちゃんさん]

Hi John,

> Does Linux guarantee a flicker-free experience with setting up PWM
> channels, or that it doesn't affect other channels?
> How do other drivers handle this situation?

I've noticed that drivers/pwm/pwm-fsl-ftm.c and
drivers/pwm/pwm-microchip-core.c use a similar approach for shared
resources between PWM channels.
They simply fail if the settings are not applicable to the hardware,
much like this driver does. However, I have yet to find a driver that
aggressively changes another channel that is already running.

Maybe you're right; Linux might not guarantee anything about this.
I think all we can do is deliver the best experience to users within
such limitations.

Best regards,
kikuchan.

[PATCH v9 0/3] Add support for Allwinner PWM on D1/T113s/R329 SoCs

[Aleksandr Shubin]

v2:
 - fix dt-bindings
 - fix a remark in the driver

v3:
 - fix dt-bindings
 - fix sunxi-d1s-t113.dtsi

v4:
 - fix a remark in the driver

v5:
 - dropped unused varibale in the driver
 - fix dt-bindings

v6:
 - add apb0 clock

v7:
 - fix a remark in the driver
 - add maintainer

v8:
 - fix compile driver for 6.8-rc

v9:
 - fix a remark in the driver
 - fix dt-bindings
 - rename apb0 -> apb

Aleksandr Shubin (3):
  dt-bindings: pwm: Add binding for Allwinner D1/T113-S3/R329 PWM
    controller
  pwm: Add Allwinner's D1/T113-S3/R329 SoCs PWM support
  riscv: dts: allwinner: d1: Add pwm node

 .../bindings/pwm/allwinner,sun20i-pwm.yaml    |  84 ++++
 .../boot/dts/allwinner/sunxi-d1s-t113.dtsi    |  12 +
 drivers/pwm/Kconfig                           |  10 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-sun20i.c                      | 387 ++++++++++++++++++
 5 files changed, 494 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
 create mode 100644 drivers/pwm/pwm-sun20i.c

-- 
2.25.1

Re: [PATCH v9 0/3] Add support for Allwinner PWM on D1/T113s/R329 SoCs

[Chris Morgan]

On Mon, May 20, 2024 at 09:42:18PM +0300, Aleksandr Shubin wrote:
> 
> v2:
>  - fix dt-bindings
>  - fix a remark in the driver
> 
> v3:
>  - fix dt-bindings
>  - fix sunxi-d1s-t113.dtsi
> 
> v4:
>  - fix a remark in the driver
> 
> v5:
>  - dropped unused varibale in the driver
>  - fix dt-bindings
> 
> v6:
>  - add apb0 clock
> 
> v7:
>  - fix a remark in the driver
>  - add maintainer
> 
> v8:
>  - fix compile driver for 6.8-rc
> 
> v9:
>  - fix a remark in the driver
>  - fix dt-bindings
>  - rename apb0 -> apb
> 
> Aleksandr Shubin (3):
>   dt-bindings: pwm: Add binding for Allwinner D1/T113-S3/R329 PWM
>     controller
>   pwm: Add Allwinner's D1/T113-S3/R329 SoCs PWM support
>   riscv: dts: allwinner: d1: Add pwm node
> 
>  .../bindings/pwm/allwinner,sun20i-pwm.yaml    |  84 ++++
>  .../boot/dts/allwinner/sunxi-d1s-t113.dtsi    |  12 +
>  drivers/pwm/Kconfig                           |  10 +
>  drivers/pwm/Makefile                          |   1 +
>  drivers/pwm/pwm-sun20i.c                      | 387 ++++++++++++++++++
>  5 files changed, 494 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
>  create mode 100644 drivers/pwm/pwm-sun20i.c
> 
> -- 
> 2.25.1
> 

Has there been any futher updates on this? It looks like v9 was last
updated in May. I only ask because I think future patches adding
support for the H700 require this as a pre-requisite.

Thank you,
Chris