[ EXTERNAL EMAIL ]
On Tue 24 Sep 2024 at 18:27, Chuan Liu <chuan.liu@xxxxxxxxxxx> wrote:
On 2024/9/24 16:50, Jerome Brunet wrote:Whatever was feeding the audio fifo is gone by that stage and fifo, tdm
[ EXTERNAL EMAIL ]
On Fri 20 Sep 2024 at 16:13, Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@xxxxxxxxxx> wrote:
From: Chuan Liu <chuan.liu@xxxxxxxxxxx>Apparently you messed something up with b4 ...
The hardware property of PLL determines that PLL can only be enabled
after PLL has been initialized. If PLL is not initialized, the
corresponding lock bit will not be set to 1, resulting in
meson_clk_pll_is_enabled() returning "false".
Therefore, if PLL is already enabled, there is no need to repeat
initialization, and the judgment "CLK_MESON_PLL_NOINIT_ENABLED" in
meson_clk_pll_init() appears redundant.
emmmm... I'm not familiar with this tool😂
---
The hardware property of PLL determines that PLL can only be enabled
after PLL has been initialized. If PLL is not initialized, the
corresponding lock bit will not be set to 1, resulting in
meson_clk_pll_is_enabled() returning "false".
Therefore, if PLL is already enabled, there is no need to repeat
initialization, and the judgment "CLK_MESON_PLL_NOINIT_ENABLED" in
meson_clk_pll_init() appears redundant.
You have a point, but we do get this kind of situation all the time:
For example, hifi_pll provides a clock for audio, which needs to be
configured
in the bootloader phase in order to play audio as soon as possible after
boot.
After entering the kernel, the hifi_pll frequency may be dynamically
adjusted
and everything will be reset as soon the audio drivers are probed.
I hardly see how keeping the PLL init through boot could apply to audio,
especially with the use of assigned-rate in DT.
(to match the audio bit rate/audio and video synchronization, etc.). TheHDMI, you presumably want to avoid a glitch in video until you're ready
gp_pll
that provides the clock for eMMC and the hdmi_pll that provides the clock
for
HDMI are all configured during the bootloader phase and cannot be configured
as RO in the kernel.
to reconfigure the pipeline. That would be a valid use-case for
CLK_MESON_PLL_NOINIT_ENABLED.
I understood your idea the first time around. I still do not agree.
My idea is to still describe the init_regs information in the kernel in the
driver:
1) If the bootloader is not enabled, the PLL will be judged as unused
during the
bootloader phase, and then enter the kernel for initialization.
2) If the bootloader has enabled PLL, in order to ensure clock continuity
after
entering the kernel, it will not repeat initialization (re-initialization
may cause the
module that references PLL to work abnormally).
Use CLK_MESON_PLL_NOINIT_ENABLED if you must keep an enabled PLL and
justify the use with a proper comment. For eMMC I'm not convinced.
Can the coupling between bootloader and kernel be avoided on the premise of--
ensuring functional integrity.
If the PLL is enabled, it has been initiallized, to some extent
yes. However we have no idea what the setting was. In general, I really
don't like inheriting settings from bootloader. It brings all sorts of
issues depending on the bootloader origin and version used by the
specific platform.
So in general a PLL should be re-initialized when possible. When it is
not possible, in most case it means the PLL should be RO and linux
should just use it.
Someone brought a specific case in between, where they needed to keep
the PLL on boot, but still be able to relock it later on. The flag
properly identify those PLLs. Much like CLK_IS_CRITICAL or
CLK_IGNORE_UNUSED, each usage shall be properly documented.
In actual application scenarios, PLL configuration is determined duringThe vast majority of those PLL should be RO then.
the bootloader phase. If PLL has been configured during the bootloader
phase, you need to add a flag to the kernel to avoid PLL
re-initialization, which will increase the coupling between the kernel
and the bootloader.
If you can relock it, you should be able to re-init it as well.
re-init may cause glitch in the PLL, which affects module work at later PLL
levels.
Signed-off-by: Chuan Liu <chuan.liu@xxxxxxxxxxx>I'm not OK with this.
---
drivers/clk/meson/clk-pll.c | 3 +--
drivers/clk/meson/clk-pll.h | 1 -
2 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
index 89f0f04a16ab..8df2add40b57 100644
--- a/drivers/clk/meson/clk-pll.c
+++ b/drivers/clk/meson/clk-pll.c
@@ -316,8 +316,7 @@ static int meson_clk_pll_init(struct clk_hw *hw)
* Keep the clock running, which was already initialized and enabled
* from the bootloader stage, to avoid any glitches.
*/
- if ((pll->flags & CLK_MESON_PLL_NOINIT_ENABLED) &&
- meson_clk_pll_is_enabled(hw))
+ if (meson_clk_pll_is_enabled(hw))
return 0;
if (pll->init_count) {--
diff --git a/drivers/clk/meson/clk-pll.h b/drivers/clk/meson/clk-pll.h
index 949157fb7bf5..cccbf52808b1 100644
--- a/drivers/clk/meson/clk-pll.h
+++ b/drivers/clk/meson/clk-pll.h
@@ -28,7 +28,6 @@ struct pll_mult_range {
}
#define CLK_MESON_PLL_ROUND_CLOSEST BIT(0)
-#define CLK_MESON_PLL_NOINIT_ENABLED BIT(1)
struct meson_clk_pll_data {
struct parm en;
---
base-commit: 0ef513560b53d499c824b77220c537eafe1df90d
change-id: 20240918-optimize_pll_flag-678a88d23f82
Best regards,
Jerome
Jerome