Message-ID: <9751014d-926e-4d42-b8e1-5a4d3e734457@amlogic.com>
Date: Fri, 31 Oct 2025 23:09:13 +0800
From: Chuan Liu <chuan.liu@...ogic.com>
To: Jerome Brunet <jbrunet@...libre.com>,
 Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@...nel.org>
Cc: Neil Armstrong <neil.armstrong@...aro.org>,
 Michael Turquette <mturquette@...libre.com>, Stephen Boyd
 <sboyd@...nel.org>, Kevin Hilman <khilman@...libre.com>,
 Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
 linux-amlogic@...ts.infradead.org, linux-clk@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 0/4] clk: amlogic: optimize the PLL driver

Hi Jerome,

On 10/31/2025 5:04 PM, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> On Fri 31 Oct 2025 at 16:10, Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@...nel.org> wrote:
> 
>> This patch series consists of four topics involving the amlogic PLL
>> driver:
>> - Fix out-of-range PLL frequency setting.
>> - Improve the issue of PLL lock failures.
>> - Add handling for PLL lock failure.
>> - Optimize PLL enable timing.
>>
>> For easier review and management, these are submitted as a single
>> patch series.
>>
>> The PLL timing optimization changes were merged into our internal
>> repository quite some time ago and have been verified on a large
>> number of SoCs:
>> - Already supported upstream: G12A, G12B, SM1, S4, A1, C3.
>> - Planned for upstream support: T7, A5, A4, S7, S7D, S6, etc.
>>
>> Based on the upstream code base, I have performed functional testing
>> on G12A, A1, A5, A4, T7, S7, S7D, and S6, all of which passed.
>>
>> Additionally, stress testing using scripts was conducted on A5 and
>> A1, with over 40,000 and 50,000 iterations respectively, and no
>> abnormalities were observed. Below is a portion of the stress test
>> log (CLOCK_ALLOW_WRITE_DEBUGFS has been manually enabled):
> 
> Okay, this little game has been going on long enough.
> 
> You've posted v2 24h hours ago
> You've got feedback within hours
> There was still a 1 question pending
> The rest of community had no chance to review.
> 

There might be a serious misunderstanding here.

In recent years, we've mainly been maintaining our code in our
internal repository, which has led to some differences between our
internal codebase and the upstream version. The patches that account
for these differences are already queued for submission, and several
SoCs are also waiting in line to be submitted. As a result, quite a
few patches have piled up, waiting to go upstream.

Previously, I had been waiting for your clock driver restructuring
patches to be ready (which have recently been merged), so for almost
a year, we haven't made much progress on clock driver–related
upstreaming.

Other drivers that depend on the clock driver have also been blocked.
This situation has been very stressful for me. I've been doing my
best to update versions as soon as possible in response to review
feedback.

> and yet, here the v3 already ! still with bearing pr_warn().
> 

Regarding pr_warn(), I explained in a previous email why I didn't
choose to use pr_warn_once(). The quick iteration of versions wasn't
an intentional disregard of your suggestions.

It's just that the upstream submission process has been severely
jammed. I admit I've been anxious, because at the previous pace, our
chips have already been in mass production for quite some time while
our clock driver still hasn't been submitted upstream, which puts us
in a difficult position, so please understand.

> Chuan, the community is not dedicated to reviewing your submission.
> Time and time again you ignore the feedback provided in reviews and the
> documentation. I've had enough of your sloppy submission.
> 
> I will not review or apply anything from you in this cycle.
> 
> You have been warned multiple times. Every time you ignore a review,
> you'll get ignored in return. This is how it will be from now on.
> 

Seeing your message was really disheartening. I want to clarify that
I've always been very grateful to and respectful of you, as well as
Neil, Martin, and others. Throughout the upstreaming process, your
feedback has been extremely helpful.

I've always taken your reviews seriously and communicate frendly. I
truly never meant to ignore your comments, please don't misunderstand.

>>
>> - For A5:
>>    # echo 491520000 > /sys/kernel/debug/clk/hifi_pll/clk_rate
>>    # cnt=0
>>    # while true; do
>>    >     echo "------------ cnt=$cnt -----------"
>>    >     echo 1 > /sys/kernel/debug/clk/hifi_pll/clk_prepare_enable
>>    >     en=$(cat /sys/kernel/debug/clk/hifi_pll/clk_prepare_enable 2>/dev/null)
>>    >     if [ "$en" != "1" ]; then
>>    >         echo "[ERROR] PLL enable test failed! (clk_prepare_enable=$en)"
>>    >         break
>>    >     fi
>>    >
>>    >     echo 0 > /sys/kernel/debug/clk/hifi_pll/clk_prepare_enable
>>    >     cnt=$((cnt + 1))
>>    >     echo -e "sleep time: 1 s."
>>    >     sleep 1
>>    > done &
>>    # ------------ cnt=0 -----------
>>    sleep time: 1 s.
>>    ------------ cnt=1 -----------
>>    sleep time: 1 s.
>>    ------------ cnt=2 -----------
>>    sleep time: 1 s.
>>    ...
>>    ------------ cnt=42076 -----------
>>    sleep time: 1 s.
>>
>> - For A1:
>>    # echo 983040000 > /sys/kernel/debug/clk/hifi_pll/clk_rate
>>    # cnt=0
>>    # while true; do
>>    >     echo "------------ cnt=$cnt -----------"
>>    >     echo 1 > /sys/kernel/debug/clk/hifi_pll/clk_prepare_enable
>>    >     en=$(cat /sys/kernel/debug/clk/hifi_pll/clk_prepare_enable 2>/dev/null)
>>    >     if [ "$en" != "1" ]; then
>>    >         echo "[ERROR] PLL enable test failed! (clk_prepare_enable=$en)"
>>    >         break
>>    >     fi
>>    >
>>    >     echo 0 > /sys/kernel/debug/clk/hifi_pll/clk_prepare_enable
>>    >     cnt=$((cnt + 1))
>>    >     echo -e "sleep time: 1 s."
>>    >     sleep 1
>>    > done &
>>    # ------------ cnt=0 -----------
>>    sleep time: 1 s.
>>    ------------ cnt=1 -----------
>>    sleep time: 1 s.
>>    ------------ cnt=2 -----------
>>    sleep time: 1 s.
>>    ...
>>    ------------ cnt=55051 -----------
>>    sleep time: 1 s.
>>
>> Signed-off-by: Chuan Liu <chuan.liu@...ogic.com>
>> ---
>> Changes in v3:
>> - Fix some formatting issues.
>> - Move the 20 us delay after reset into the corresponding if
>> condition (no delay is needed if there is no reset).
>> - Move the code that releases rst back to execute before current_en.
>> - Remove the patch that changes the active level of l_detect.
>> - Link to v2: https://lore.kernel.org/r/20251030-optimize_pll_driver-v2-0-37273f5b25ab@amlogic.com
>>
>> Changes in v2:
>> - Modify the judgment condition of 'm' out of range.
>> - Split the PLL timing optimization patch to make it easier to review.
>> - Link to v1: https://lore.kernel.org/r/20251022-optimize_pll_driver-v1-0-a275722fb6f4@amlogic.com
>>
>> ---
>> Chuan Liu (4):
>>        clk: amlogic: Fix out-of-range PLL frequency setting
>>        clk: amlogic: Improve the issue of PLL lock failures
>>        clk: amlogic: Add handling for PLL lock failure
>>        clk: amlogic: Optimize PLL enable timing
>>
>>   drivers/clk/meson/clk-pll.c | 64 +++++++++++++++++++++++++++------------------
>>   1 file changed, 39 insertions(+), 25 deletions(-)
>> ---
>> base-commit: 01f3a6d1d59b8e25a6de243b0d73075cf0415eaf
>> change-id: 20251020-optimize_pll_driver-7bef91876c41
>>
>> Best regards,
> 
> --
> Jerome

Powered by blists - more mailing lists