Re: [PATCH] pwm: meson: add support for S4 chip family

[Heiner Kallweit <hkallweit1@gmail.com>]

On 27.03.2023 19:50, Uwe Kleine-König wrote:
> On Mon, Mar 27, 2023 at 07:00:35PM +0200, Heiner Kallweit wrote:
>> The best (for achieving best precision) input frequency is 0xffff / period.
>> So we should do our best to come as close as possible to this frequency.
>> By using set_rate() CCF will choose the optimal mux input and divider value.
> 
> Just for the record: Here might be a misconception. There is (AFAIK) no
> reason to believe that set_rate would pick an optimal configuration. And
> if it's only because there is no objective definition of "optimal". For
> example if a driver for an SD card requests 400MHz, it might not want
> anything faster and so probably prefers 202 MHz over 404 MHz. However a
> driver for a UART would prefer the 404 MHz in this case. IMHO we'd need
> something like
> 
Right, I wasn't precise enough. In my case optimal is "fastest rate <= requested rate".
This seems to be the behavior of clk core default implementations like
clk_mux_determine_rate_flags() an divider_determine_rate(), used by the meson
clock drivers.
However in general clock providers can provide their own determine_rate()
implementations. And, as a disclaimer, I have to admit that I'm no expert in CCF.

> 	clk_rounddown_rate();
> 	clk_roundnear_rate(); and maybe
> 	clk_roundup_rate()
> 
> to please all drivers. But that sounds like a bigger quest.
> 
> If your clk can provide (say) 1000000 Hz and 2000000 Hz (and nothing in
> between) there is no rule in the CCF that tells you which of the two to
> pick if you request 1000001 Hz or 1999999 Hz. (The only thing I would
> not expect is that you get 1000000 Hz when requesting 1999999 Hz *and*
> 2000000 Hz when requesting 1000001 Hz.)
> 
>> Not sure why we should restrict ourselves to one mux parent only.
>> E.g. for a very low duty cycle a higher input frequency than the xtal 24MHz may be preferable.
>>
>> Not only the mux is outside the PWM block now, also the divider (8 bit wide instead of 7 bit
>> as of today). So we need a set_rate() anyway to set the divider value.
>> What I can't say is whether the internal divider still exists (then external and internal
>> divider would be cascaded) or is removed or bypassed.
>> It seems like when adding the external PWM clock feature Amlogic forgot to update
>> the PWM block documentation, as there's no reference at all to the now external input clock
>> (except in the clocks section).
> 
> It would be great to test (or ask?) if the internal divider still exists
> and do the right thing then.
> 
> Having said that, I wonder if it would make sense to rework the driver
> for the variants with the internal mux to also abstract the divider as a
> proper clk.
> 
I think this would make sense. Then we could ignore the mux parent set in DT
and let CCF select the mux parent and divider value. And we could use one
pwm calc logic and get rid of most if(ext_clk).


> Best regards
> Uwe
>