Re: [PATCH v4 4/4] pwm: meson: make full use of common clock framework

[Heiner Kallweit <hkallweit1@gmail.com>]

On 19.05.2023 17:30, Dmitry Rokosov wrote:
> Hello Heiner,
> 
> Thank you for the patch series!
> 
> I am currently working on the Amlogic A1 clock driver and other
> peripheral devices, including PWM. During a discussion about the clock
> driver with Martin Blumenstingl, we found an intersection between the
> clock driver and your PWM CCF support patch series. Please see my
> comments below.
> 
> On Thu, Apr 13, 2023 at 07:54:46AM +0200, Heiner Kallweit wrote:
>> Newer versions of the PWM block use a core clock with external mux,
>> divider, and gate. These components either don't exist any longer in
>> the PWM block, or they are bypassed.
>> To minimize needed changes for supporting the new version, the internal
>> divider and gate should be handled by CCF too.
>>
>> I didn't see a good way to split the patch, therefore it's somewhat
>> bigger. What it does:
>>
>> - The internal mux is handled by CCF already. Register also internal
>>   divider and gate with CCF, so that we have one representation of the
>>   input clock: [mux] parent of [divider] parent of [gate]
>>   
>> - Now that CCF selects an appropriate mux parent, we don't need the
>>   DT-provided default parent any longer. Accordingly we can also omit
>>   setting the mux parent directly in the driver.
>>   
>> - Instead of manually handling the pre-div divider value, let CCF
>>   set the input clock. Targeted input clock frequency is
>>   0xffff * 1/period for best precision.
>>   
>> - For the "inverted pwm disabled" scenario target an input clock
>>   frequency of 1GHz. This ensures that the remaining low pulses
>>   have minimum length.
>>
>> I don't have hw with the old PWM block, therefore I couldn't test this
>> patch. With the not yet included extension for the new PWM block
>> (channel->clk coming directly from get_clk(external_clk)) I didn't
>> notice any problem. My system uses PWM for the CPU voltage regulator
>> and for the SDIO 32kHz clock.
>>
>> Note: The clock gate in the old PWM block is permanently disabled.
>> This seems to indicate that it's not used by the new PWM block.
>>
>> Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> Changes to RFT/RFC version:
>> - use parent_hws instead of parent_names for div/gate clock
>> - use devm_clk_hw_register where the struct clk * returned by
>>   devm_clk_register isn't needed
>>
>> v2:
>> - add patch 1
>> - add patch 3
>> - switch to using clk_parent_data in all relevant places
>> v3:
>> - add flag CLK_IGNORE_UNUSED
>> v4:
>> - remove variable tmp in meson_pwm_get_state
>> - don't use deprecated function devm_clk_register
> 
> [...]
> 
>> @@ -166,7 +158,11 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
>>  	if (state->polarity == PWM_POLARITY_INVERSED)
>>  		duty = period - duty;
>>  
>> -	fin_freq = clk_get_rate(channel->clk);
>> +	freq = div64_u64(NSEC_PER_SEC * (u64)0xffff, period);
>> +	if (freq > ULONG_MAX)
>> +		freq = ULONG_MAX;
>> +
>> +	fin_freq = clk_round_rate(channel->clk, freq);
>>  	if (fin_freq == 0) {
>>  		dev_err(meson->chip.dev, "invalid source clock frequency\n");
>>  		return -EINVAL;
> 
> As mentioned previously, we have discussed one optimization for PWM
> parent clock calculation. Many modern Amlogic SoCs include an RTC clock
> within the clock tree. This clock provides a stable and efficient 32kHz
> input for several clock objects that can be inherited through the muxes
> from the RTC clock.
> 
> In short, we aim to use the RTC clock parent directly for PWM to
> generate a 32kHz clock on the PWM lines. Martin has suggested one way to
> do so, which is described in [0]. You can also refer to our IRC
> discussion in [1].
> 
> I would appreciate your thoughts on this. Please let me know what you
> think.
> 

Requesting a frequency of (NSEC_PER_SEC * 0xffffULL / period) is based
on the assumption that the highest possible input frequency always
allows to generate a period that is close enough to the requested period.

To find the best parent you'd need a somewhat more complex logic outside CCF.
What you want is the parent where (f_parent * period / NSEC_PER_SEC) is
closest to an integer in the range 1 .. 0xffff.
IOW: max(abs((f_parent * period) % 10^9 - 5 * 10^8))

This can be done, question is whether it's needed and worth the effort.

This would be the generic solution. If you just want to handle the case
that period 1/32.768Hz is requested, an adjusted version of Martins's
pseudo-code formula should do.
Best I think as follow-up to my series.

> [...]
> 
> Links:
>     [0] https://lore.kernel.org/all/CAFBinCCPf+asVakAxeBqV-jhsZp=i2zbShByTCXfYYAQ6cCnHg@mail.gmail.com/
>     [1] https://libera.irclog.whitequark.org/linux-amlogic/2023-05-18
>