Received-SPF: pass (google.com: domain of linux-kernel+bounces-192825-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Tue, 28 May 2024 20:41:44 +0300
From: Dmitry Rokosov <ddrokosov@salutedevices.com>
To: <neil.armstrong@linaro.org>, <jbrunet@baylibre.com>,
	<mturquette@baylibre.com>, <sboyd@kernel.org>, <robh+dt@kernel.org>,
	<krzysztof.kozlowski+dt@linaro.org>, <khilman@baylibre.com>,
	<martin.blumenstingl@googlemail.com>
CC: <jian.hu@amlogic.com>, <kernel@sberdevices.ru>, <rockosov@gmail.com>,
	<linux-amlogic@lists.infradead.org>, <linux-clk@vger.kernel.org>,
	<devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v3 0/7] clk: meson: introduce Amlogic A1 SoC Family CPU
 clock controller driver
Message-ID: <20240528174144.v2aiq4zryqdbu6dh@CAB-WSD-L081021>
References: <20240515185103.20256-1-ddrokosov@salutedevices.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240515185103.20256-1-ddrokosov@salutedevices.com>
User-Agent: NeoMutt/20220415

Hello guys!

Kindly reminder :)

On Wed, May 15, 2024 at 09:47:23PM +0300, Dmitry Rokosov wrote:
> The CPU clock controller plays a general role in the Amlogic A1 SoC
> family by generating CPU clocks. As an APB slave module, it offers the
> capability to inherit the CPU clock from two sources: the internal fixed
> clock known as 'cpu fixed clock' and the external input provided by the
> A1 PLL clock controller, referred to as 'syspll'.
> 
> It is important for the driver to handle the cpu_clk rate switching
> effectively by transitioning to the CPU fixed clock to avoid any
> potential execution freezes.
> 
> Validation:
> * to double-check all clk flags, run the below helper script:
> 
> ```
> pushd /sys/kernel/debug/clk
> for f in *; do
>     if [[ -f "$f/clk_flags" ]]; then
>         flags="$(cat $f/clk_flags | awk '{$1=$1};1' | sed ':a;N;$!ba;s/\n/ | /g')"
>         echo -e "$f: $flags"
>     fi
> done
> popd
> ```
> 
> * to trace the current clks state, use the
>   '/sys/kernel/debug/clk/clk_dump' node with jq post-processing:
> 
> ```
> $ cat /sys/kernel/debug/clk/clk_dump | jq '.' > clk_dump.json
> ```
> 
> * to see the CPU clock hierarchy, use the
> '/sys/kernel/debug/clk/clk_summary' node with jq post-processing:
> 
> ```
> $ cat /sys/kernel/debug/clk/clk_summary | jq '.' > clk_dump.json
> ```
> 
> when cpu_clk is inherited from sys_pll, it should be:
> 
> ```
> syspll_in    1  1  0  24000000    0  0  50000  Y  deviceless                 no_connection_id
>   sys_pll    2  2  0  1200000000  0  0  50000  Y  deviceless                 no_connection_id
>     cpu_clk  1  1  0  1200000000  0  0  50000  Y  cpu0                       no_connection_id
>                                                   cpu0                       no_connection_id
>                                                   fd000000.clock-controller  dvfs
>                                                   deviceless                 no_connection_id
> ```
> 
> and from cpu fixed clock:
> 
> ```
> fclk_div3_div           1  1  0  512000000  0  0  50000  Y  deviceless                 no_connection_id
>   fclk_div3             4  4  0  512000000  0  0  50000  Y  deviceless                 no_connection_id
>     cpu_fsource_sel0    1  1  0  512000000  0  0  50000  Y  deviceless                 no_connection_id
>       cpu_fsource_div0  1  1  0  128000000  0  0  50000  Y  deviceless                 no_connection_id
>         cpu_fsel0       1  1  0  128000000  0  0  50000  Y  deviceless                 no_connection_id
>           cpu_fclk      1  1  0  128000000  0  0  50000  Y  deviceless                 no_connection_id
>             cpu_clk     1  1  0  128000000  0  0  50000  Y  cpu0                       no_connection_id
>                                                             cpu0                       no_connection_id
>                                                             fd000000.clock-controller  dvfs
>                                                             deviceless                 no_connection_id
> ```
> 
> * to debug cpu clk rate propagation and proper parent switching, compile
>   kernel with the following definition:
>     $ sed -i "s/undef CLOCK_ALLOW_WRITE_DEBUGFS/define CLOCK_ALLOW_WRITE_DEBUGFS/g" drivers/clk/clk.c
>   after that, clk_rate debug node for each clock will be available for
>   write operation
> 
> Changes v3 since v2 at [2]:
>     - rename CLK_MESON_PLL_INIT_ONCE to CLK_MESON_PLL_NOINIT_ENABLED to
>       accurately describe the behavior when we don't run the
>       initialization sequence for an already enabled PLL
>     - provide accurate comment about CLK_MESON_PLL_NOINIT_ENABLED flag
>       to meson_clk_pll_init() and A1 sys_pll clock definition
>     - tag syspll_in and sys_pll input clocks as optional in the a1-pll
>       and a1-peripherals clkc bindings per Conor and Rob suggestion
>     - move sys_pll_div16 clock from a1-pll clkc to a1-peripherals clkc
>       as Jerome suggested
> 
> Changes v2 since v1 at [1]:
>     - introduce new 'INIT_ONCE' flag to eliminate init for already
>       enabled PLL
>     - explain why we need to break ABI for a1-pll driver by adding
>       sys_pll connections
>     - implement sys_pll init sequence, which is applicable when sys_pll
>       is disabled
>     - remove CLK_IS_CRITICAL from sys_pll
>     - move sys_pll_div16 binding to the end per Rob's suggestion
>     - add Rob's RvB
>     - remove holes from the beginning of the cpu clock controller regmap
>     - move a1-cpu.h registers offsets definition to a1-cpu.c
>     - set CLK_SET_RATE_GATE for parallel cpu fixed clock source trees
>       per Martin's and Jerome's suggestion
>     - redesign clock notifier block from cpu_clk to sys_pll to keep
>       cpu_clock working continuously (the same implementation is located
>       in the g12a clock driver)
> 
> Links:
>     [1] https://lore.kernel.org/all/20240329205904.25002-1-ddrokosov@salutedevices.com/
>     [2] https://lore.kernel.org/all/20240510090933.19464-1-ddrokosov@salutedevices.com/
> 
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> 
> Dmitry Rokosov (7):
>   clk: meson: add 'NOINIT_ENABLED' flag to eliminate init for enabled
>     PLL
>   dt-bindings: clock: meson: a1: pll: introduce new syspll bindings
>   clk: meson: a1: pll: support 'syspll' general-purpose PLL for CPU
>     clock
>   dt-bindings: clock: meson: a1: peripherals: support sys_pll input
>   clk: meson: a1: peripherals: support 'sys_pll_div16' clock as GEN
>     input
>   dt-bindings: clock: meson: add A1 CPU clock controller bindings
>   clk: meson: a1: add Amlogic A1 CPU clock controller driver
> 
>  .../bindings/clock/amlogic,a1-cpu-clkc.yaml   |  64 ++++
>  .../clock/amlogic,a1-peripherals-clkc.yaml    |   9 +-
>  .../bindings/clock/amlogic,a1-pll-clkc.yaml   |   9 +-
>  drivers/clk/meson/Kconfig                     |  10 +
>  drivers/clk/meson/Makefile                    |   1 +
>  drivers/clk/meson/a1-cpu.c                    | 331 ++++++++++++++++++
>  drivers/clk/meson/a1-peripherals.c            |  18 +-
>  drivers/clk/meson/a1-pll.c                    |  72 ++++
>  drivers/clk/meson/a1-pll.h                    |   6 +
>  drivers/clk/meson/clk-pll.c                   |  40 ++-
>  drivers/clk/meson/clk-pll.h                   |   1 +
>  .../dt-bindings/clock/amlogic,a1-cpu-clkc.h   |  19 +
>  .../clock/amlogic,a1-peripherals-clkc.h       |   1 +
>  .../dt-bindings/clock/amlogic,a1-pll-clkc.h   |   1 +
>  14 files changed, 560 insertions(+), 22 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-cpu-clkc.yaml
>  create mode 100644 drivers/clk/meson/a1-cpu.c
>  create mode 100644 include/dt-bindings/clock/amlogic,a1-cpu-clkc.h
> 
> -- 
> 2.43.0
> 

-- 
Thank you,
Dmitry