Message-ID: <160e221e-98a8-4097-8340-94ac7d208136@bootlin.com>
Date: Mon, 22 Dec 2025 10:17:07 +0100
From: Richard GENOUD <richard.genoud@...tlin.com>
To: Jernej Škrabec <jernej.skrabec@...il.com>,
 Uwe Kleine-König <u.kleine-koenig@...libre.com>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Chen-Yu Tsai <wens@...e.org>,
 Samuel Holland <samuel@...lland.org>, Philipp Zabel <p.zabel@...gutronix.de>
Cc: Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
 linux-pwm@...r.kernel.org, devicetree@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 0/4] Introduce Allwinner H616 PWM controller

Hi Jernej,

Le 21/12/2025 à 20:12, Jernej Škrabec a écrit :
> Dne sreda, 17. december 2025 ob 09:25:00 Srednjeevropski standardni čas je Richard Genoud napisal(a):
>> Allwinner H616 PWM controller is quite different from the A10 one.
>>
>> It can drive 6 PWM channels, and like for the A10, each channel has a
>> bypass that permits to output a clock, bypassing the PWM logic, when
>> enabled.
>>
>> But, the channels are paired 2 by 2, sharing a first set of
>> MUX/prescaler/gate.
>> Then, for each channel, there's another prescaler (that will be bypassed
>> if the bypass is enabled for this channel).
>>
>> It looks like that:
>>              _____      ______      ________
>> OSC24M --->|     |    |      |    |        |
>> APB1 ----->| Mux |--->| Gate |--->| /div_m |-----> PWM_clock_src_xy
>>             |_____|    |______|    |________|
>>                            ________
>>                           |        |
>>                        +->| /div_k |---> PWM_clock_x
>>                        |  |________|
>>                        |    ______
>>                        |   |      |
>>                        +-->| Gate |----> PWM_bypass_clock_x
>>                        |   |______|
>> PWM_clock_src_xy -----+   ________
>>                        |  |        |
>>                        +->| /div_k |---> PWM_clock_y
>>                        |  |________|
>>                        |    ______
>>                        |   |      |
>>                        +-->| Gate |----> PWM_bypass_clock_y
>>                            |______|
>>
>> Where xy can be 0/1, 2/3, 4/5
>>
>> PWM_clock_x/y serve for the PWM purpose.
>> PWM_bypass_clock_x/y serve for the clock-provider purpose.
>> The common clock framework has been used to manage those clocks.
>>
>> This PWM driver serves as a clock-provider for PWM_bypass_clocks.
>> This is needed for example by the embedded AC300 PHY which clock comes
>> from PMW5 pin (PB12).
> 
> No. Drop all clocks related code and make this pure PWM driver, like pwm-sun4i
> is. For AC300, AC200 or whatever other device may need clock produced by PWM,
> pwm-clock can be used like this:
> 
> ac300_pwm_clk: ac300-clk {
> 	compatible = "pwm-clock";
> 	#clock-cells = <0>;
> 	clock-frequency = <24000000>;
> 	pinctrl-names = "default";
> 	pinctrl-0 = <&pwm1_pin>;
> 	pwms = <&pwm 1 42 0>;
> };
> 
> ac300 {
> 	...
> 	clocks = <&ac300_pwm_clk>;
> 	...
> };
Yes, that was my first move, but after trying quite hard, I came to the 
conclusion that pwm-clock is not fit for this precise case.
If we had only one source clock, this would be a perfect fit though.

Here's the problem: the pwm-clock request a period from the PWM driver, 
without any clue that it actually wants a clock at a specific frequency, 
and not a PWM signal with duty cycle capability.
So, the PWM driver doesn't know if it can use the bypass or not, it 
doesn't even have the real accurate frequency information (23809524 Hz 
instead of 24MHz) because the pwm_apply only deals with periods.
With pwm-clock, we loose a precious information along the way (that we 
actually want a clock and not a PWM signal).
That's ok with simple PWM drivers that don't have multiple input clocks, 
but in this case, without this information, we can't know for sure which 
clock to use.
And here, for instance, if pwm-clock requests 42ns, the logic is to 
select the highest clock (100MHz), with no prescaler, and a duty cycle 
value of 2/4 => we have 25MHz instead of 24MHz.
And that's a perfectly fine choice for a PMW, because we still can 
change the duty cycle in the range [0-4]/4.
But obviously for a clock, we don't care about the duty cycle, but more 
about the clock accuracy.
When I tried to use pwm-clock, I ended up making assumptions on what was 
the intended use (if the asked period is 42 with a duty cycle of 50% 
maybe the consumer wants a clock), but that was too hack-ish.

That's why I put aside the pwm-clock and I went for this instead.


Thanks for the review!

Regards,
Richard

> 
> Best regards,
> Jernej
> 
>>
>> This series is based onto v6.19-rc1
>>
>> Changes since v1:
>> - rebase onto v6.19-rc1
>> - add missing headers
>> - remove MODULE_ALIAS (suggested by Krzysztof)
>> - use sun4i-pwm binding instead of creating a new one (suggested by Krzysztof)
>> - retrieve the parent clocks from the devicetree
>> - switch num_parents to unsigned int
>>
>> Richard Genoud (4):
>>    dt-bindings: pwm: allwinner: add h616 pwm compatible
>>    pwm: sun50i: Add H616 PWM support
>>    arm64: dts: allwinner: h616: add PWM controller
>>    MAINTAINERS: Add entry on Allwinner H616 PWM driver
>>
>>   .../bindings/pwm/allwinner,sun4i-a10-pwm.yaml |  19 +-
>>   MAINTAINERS                                   |   5 +
>>   .../arm64/boot/dts/allwinner/sun50i-h616.dtsi |  47 +
>>   drivers/pwm/Kconfig                           |  12 +
>>   drivers/pwm/Makefile                          |   1 +
>>   drivers/pwm/pwm-sun50i-h616.c                 | 892 ++++++++++++++++++
>>   6 files changed, 975 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/pwm/pwm-sun50i-h616.c
>>
>>
>> base-commit: 8f0b4cce4481fb22653697cced8d0d04027cb1e8
>>
> 
> 
> 
> 


-- 
Richard Genoud, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Powered by blists - more mailing lists