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

[Dmitry Rokosov <ddrokosov@sberdevices.ru>]

Heiner,

On Fri, May 19, 2023 at 06:53:30PM +0200, Heiner Kallweit wrote:
> 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.
> 

Certainly, if possible, please include this special case in the next
version of your series. Appreciate it!

[...]

-- 
Thank you,
Dmitry