Re: [PATCH v4 0/4] Introduce Allwinner H616 PWM controller

[Richard GENOUD <richard.genoud@bootlin.com>]

Le 09/04/2026 à 19:16, Paul Kocialkowski a écrit :
> Hi Richard,
> 
> On Thu 05 Mar 26, 10:19, Richard Genoud wrote:
>> Allwinner H616 PWM controller is quite different from the A10 one.
> 
> As I've mentionned before, this PWM controller is not specific to the H616
> but also appears in other chips, so the name of the driver and registers
> should not mention H616.
> 
> After further investigation, I can see multiple versions of this new PWM IP
> being used in different chips, starting with the R40/V40 (sun8iw11) in 2016.
> 
> The latest downstream BSP driver has a list of the different generations:
> https://github.com/radxa/allwinner-bsp/blob/cubie-aiot-v1.4.6/drivers/pwm/pwm-sunxi.c#L1901
> 
> We have a first generation called v100/v101 for the following chips:
> H616, R328 and R40. A second generation is called v200 and brings slight
> register layout differences for A133, D1/T113-S3 and V851. Subsequent
> iterations (v201-5) are used in more recent chips like A527 and A733 and
> seem register-compatible with v200 (from a quick look).
> 
> So what I suggest here is to rename the driver "sun8i-pwm" and eventually add
> a list of generations to the driver and different registers when needed, with
> an appropriate suffix in their name.
> 
> But since you're currently only dealing with H616, this work can be done later
> when introducing support for more chips.
ok, I'm fine with that :)

> 
>> 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).
>>
>> Usually, to get a clock from a PWM driver, we use the pwm-clock driver
>> so that the PWM driver doesn't need to be a clk-provider itself.
>> While this works in most cases, here it just doesn't.
>> That's because 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.
> 
>  From what I understand the pwm-clock driver will either assume a fixed rate
> set in device-tree or deduce the rate from the pwm period. In any case it will
> check that the pwm period (which it cannot change) is the same as the requested
> clock period.
> 
> So I agree that pwm-clock is unable to change the clock rate at runtime and will
> just use whatever frequency the pwm is running at (which is typically set
> in the device-tree consumer property).
> 
>> 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 PWM drivers only deal with periods.
> 
> I agree that the driver needs to register as a proper clock provider in
> addition to pwm. But what happens if the same PWM clock is requested both from
> the clk side and the pwm side?

The first to request it is the winner :)
The other ones will receive a -EBUSY
In h616_pwm_request() and h616_pwm_of_clk_get(), the channel mode is 
checked, and if it's free to use, it's set as either PWM or CLK mode so 
that it can't be requested a second time.

> 
>> 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 we ask for a 24MHz clock, pwm-clock will
>> requests 42ns (assigned-clocks doesn't help for that matter). 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.
>>
>> And actually, this PWM is really a PWM AND a real clock when the bypass
>> is set.
> 
> Make sense to me.
> 
>> This series is based onto v6.19-rc4
>>
>> NB: checkpatch is not happy with patch 2, but it's a false positive.
>> It doesn't detect that PWM_XY_SRC_MUX/GATE/DIV are structures, but as
>> it's more readable like that, I prefer keeping it that way.
>>
>> NB2: for geopolitical reasons, I didn't re-use the old series that Paul
>> was referring to.
>>
>> Changes since v3:
>> - gather Acked-by/Tested-by
>> - fix cast from pointer to integer of different size (kernel test robot
>>    with arc platform)
>> - add devm_action for clk_hw_unregister_composite as suggested by Philipp
>> - remove now unused pwm_remove as suggested by Philipp
>>
>> Changes since v2:
>> - use U32_MAX instead of defining UINT32_MAX
>> - add a comment on U32_MAX usage in clk_round_rate()
>> - change clk_table_div_m (use macros)
>> - fix formatting (double space, superfluous comma, extra line feed)
>> - fix the parent clock order
>> - simplify code by using scoped_guard()
>> - add missing const in to_h616_pwm_chip() and rename to
>> h616_pwm_from_chip()
>> - add/remove missing/superflous error messages
>> - rename cnt->period_ticks, duty_cnt->duty_ticks
>> - fix PWM_PERIOD_MAX
>> - add .remove() callback
>> - fix DIV_ROUND_CLOSEST_ULL->DIV_ROUND_UP_ULL
>> - add H616_ prefix
>> - protect _reg in macros
>> - switch to waveforms instead of apply/get_state
>> - shrink struct h616_pwm_channel
>> - rebase on v6.19-rc4
>>
>> 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                 | 936 ++++++++++++++++++
>>   6 files changed, 1019 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/pwm/pwm-sun50i-h616.c
>>
>>
>> base-commit: 11439c4635edd669ae435eec308f4ab8a0804808
> 

Regards,
Richard