Message-ID: <3fb39050-9543-f4a4-c8f2-a996e24c8e16@sberdevices.ru>
Date:   Wed, 24 May 2023 03:52:06 +0300
From:   George Stark <gnstark@...rdevices.ru>
To:     Heiner Kallweit <hkallweit1@...il.com>
CC:     Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
        "neil.armstrong@...aro.org" <neil.armstrong@...aro.org>,
        "jbrunet@...libre.com" <jbrunet@...libre.com>,
        "mturquette@...libre.com" <mturquette@...libre.com>,
        "sboyd@...nel.org" <sboyd@...nel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "khilman@...libre.com" <khilman@...libre.com>,
        "jian.hu@...ogic.com" <jian.hu@...ogic.com>,
        kernel <kernel@...rdevices.ru>,
        "rockosov@...il.com" <rockosov@...il.com>,
        "linux-amlogic@...ts.infradead.org" 
        <linux-amlogic@...ts.infradead.org>,
        "linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Dmitry Rokosov <DDRokosov@...rdevices.ru>
Subject: Re: [PATCH v14 6/6] clk: meson: a1: add Amlogic A1 Peripherals clock
 controller driver

On 5/22/23 23:36, Heiner Kallweit wrote:
> On 22.05.2023 15:44, Dmitry Rokosov wrote:
>> Heiner,
>>
>> On Fri, May 19, 2023 at 06:10:50PM +0200, Heiner Kallweit wrote:
>>> On 18.05.2023 22:04, Martin Blumenstingl wrote:
>>>> Hi Dmitry,
>>>>
>>>> On Wed, May 17, 2023 at 12:34 PM Dmitry Rokosov
>>>> <ddrokosov@...rdevices.ru> wrote:
>>>> [...]
>>>>>>> Additionally, the CCF determines the best ancestor based on how close
>>>>>>> its rate is to the given one, based on arithmetic calculations. However,
>>>>>>> we have independent knowledge that a certain clock would be better, with
>>>>>>> less jitter and fewer intermediaries, which will likely improve energy
>>>>>>> efficiency. Sadly, the CCF cannot take this into account.
>>>>>> I agree that the implementation in CCF is fairly simple. There's ways
>>>>>> to trick it though: IIRC if there are multiple equally suitable clocks
>>>>>> it picks the first one. For me all of this has worked so far which is
>>>>>> what makes me curious in this case (not saying that anything is wrong
>>>>>> with your approach).
>>>>>>
>>>>>> Do you have a (real world) example where the RTC clock should be
>>>>>> preferred over another clock?
>>>>>>
>>>>> Yes, a real-life example is the need for a 32Khz clock for an external
>>>>> wifi chip. There is one option to provide this clock with high
>>>>> precision, which is RTC + GENCLK.
>>>>>
>>>>>> I'm thinking about the following scenario.
>>>>>> PWM parents:
>>>>>> - XTAL: 24MHz
>>>>>> - sys: not sure - let's say 166.67MHz
>>>>>> - RTC: 32kHz
>>>>>>
>>>>>> Then after that there's a divider and a gate.
>>>>>>
>>>>>> Let's say the PWM controller needs a 1MHz clock: it can take that from
>>>>>> XTAL or sys. Since XTAL is evenly divisible to 1MHz CCF will pick that
>>>>>> and use the divider.
>>>>>> But let's say the PWM controller needs a 32kHz clock: CCF would
>>>>>> automatically pick the RTC clock.
>>>>>> So is your implementation there to cover let's say 1kHz where
>>>>>> mathematically 24MHz can be divided evenly to 1kHz (and thus should
>>>>>> not result in any jitter) but RTC gives better precision in the real
>>>>>> world (even though it's off by 24Hz)?
>>>>>>
>>>>> I don't think so. The highest precision that RTC can provide is from a
>>>>> 32KHz rate only. However, I believe that a 1kHz frequency can also be
>>>>> achieved by using xtal 24MHz with a divider, which can provide high
>>>>> precision as well.
>>>> Thank you again for the great discussion on IRC today.
>>>> Here's my short summary so I don't forget before you'll follow up on this.
>>>>
>>>> In general there's two known cases where the RTC clock needs to be used:
>>>> a) When using the GENCLK output of the SoC to output the 32kHz RTC
>>>> clock and connect that to an SDIO WiFi chip clock input (this seems
>>>> useful in my understanding because the RTC clock provides high
>>>> precision)
>>>> b) When using the PWM controller to output a 32kHz clock signal. In
>>>> this case my understanding is that using the RTC clock as input to the
>>>> PWM controller results in the best possible signal
>>>>
>>>> The second case won't be supported with Heiner's patches [0] that use
>>>> CCF (common clock framework) in the PWM controller driver.
>>>> In this series the parent clock is calculated using:
>>>>    freq = div64_u64(NSEC_PER_SEC * (u64)0xffff, period);
>>>>
>>>> A 32kHz clock means a PWM period of 30518ns. So with the above
>>> To be precise: 30517,578125ns
>>> What means that the PWM framework can't say "I want 32768Hz",
>>> but just "I want something being very close to 32768Hz".
>>> So what you need is some simple heuristic to interpret the
>>> PWM request -> "PWM requests 30518ns, but supposedly it wants
>>> 32768Hz"
>>>
>>> NSEC_PER_SEC / 30518 = 32767 (rounded down from 32767,547)
>>> clk_round_rate(channel->clk, 32767) would return 0 (I *think*),
>>> because it tries to find the next lower clock.
>>>
>>> The SoC families I'm familiar with have fclkin2 as PWM parent.
>>> That's 1 GHz in my case, what results in a frequency of 32.767,547Hz
>>> for period = 30518n.
>>> What you're saying is that newer generations don't have PWM parents
>>>> 24MHz any longer?
>> No, of course not. For example, a fixed PLL (with all fclk_divX
>> settings) has rates higher than 24MHz. However, we need to consider the
>> 'heavy' background of such PWM.
>>
>> However, we have a "lightweight" clkin (special rtc32k) with a rate of
>> 32kHz that we could potentially use as an input to produce a 32kHz
>> output on the PWM lines. I don't see any reason why we should not
>> support such special cases.
>>
> Two more things to consider:
> 1. When wanting a 32kHz (well, 32768Hz) output with a 50% duty cycle,
>     then we need hi=0 and lo=0 with a 64kHz input clock.
>     See point 2 for an explanation of why 0 and not 1.
>     Means we couldn't use the RTC input clock. Did you consider this?
>     Or do I miss something?
> 2. Seems the PWM block internally increments hi and lo, except the
>     constant_en bit is set on newer PWM block versions.
>     For bigger cnt values the impact is negligible, but for very small
>     values it's something we have to consider.
>     This was one additional motivation for me to choose an input
>     frequency that creates big cnt values.
>
Hello Heiner

As I mentioned earlier I have some changes to take into account lo and hi regs incrementing.

But it's more convenient to base my patch on top on one of yours (https://lore.kernel.org/linux-amlogic/23fe625e-dc23-4db8-3dce-83167cd3b206@gmail.com/)

Is that ok if I resend your patch along with mine in series?

Best regards
George

>>>
>>>> calculation the PWM driver is asking for a clock rate of >=2GHz.
>>>> We concluded that letting the common clock framework choose the best
>>>> possible parent (meaning: removing CLK_SET_RATE_NO_REPARENT here) can
>>>> be a way forward.
>>>> But this means that the PWM controller driver must try to find the
>>>> best possible parent somehow. The easiest way we came up with
>>>> (pseudo-code):
>>>>    freq = NSEC_PER_SEC / period;
>>>>    fin_freq = clk_round_rate(channel->clk, freq);
>>>>    if (fin_freq != freq) {
>>>>      freq = div64_u64(NSEC_PER_SEC * (u64)0xffff, period);
>>>>      fin_freq = clk_round_rate(channel->clk, freq);
>>>>    }
>>>>
>>>> The idea is: for a requested 32kHz signal the PWM period is 30518ns.
>>>> The updated logic would find that there's a matching clock input and
>>>> use that directly. If not: use the original logic as suggested by
>>>> Heiner.
>>>>
>>>>
>>>> Best regards,
>>>> Martin
>>>>
>>>>
>>>> [0] https://lore.kernel.org/linux-amlogic/9faca2e6-b7a1-4748-7eb0-48f8064e323e@gmail.com/
>

Powered by blists - more mailing lists