* [PATCH] clk: meson: pll: Update the meson_clk_pll_init execution judgment logic @ 2024-09-20 8:13 Chuan Liu via B4 Relay 2024-09-24 8:50 ` Jerome Brunet 0 siblings, 1 reply; 5+ messages in thread From: Chuan Liu via B4 Relay @ 2024-09-20 8:13 UTC (permalink / raw) To: Neil Armstrong, Jerome Brunet, Michael Turquette, Stephen Boyd, Kevin Hilman, Martin Blumenstingl Cc: linux-amlogic, linux-clk, linux-arm-kernel, linux-kernel, Chuan Liu From: Chuan Liu <chuan.liu@amlogic.com> 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. --- 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. In actual application scenarios, PLL configuration is determined during 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. Signed-off-by: Chuan Liu <chuan.liu@amlogic.com> --- 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, -- Chuan Liu <chuan.liu@amlogic.com> _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] clk: meson: pll: Update the meson_clk_pll_init execution judgment logic 2024-09-20 8:13 [PATCH] clk: meson: pll: Update the meson_clk_pll_init execution judgment logic Chuan Liu via B4 Relay @ 2024-09-24 8:50 ` Jerome Brunet 2024-09-24 10:27 ` Chuan Liu 0 siblings, 1 reply; 5+ messages in thread From: Jerome Brunet @ 2024-09-24 8:50 UTC (permalink / raw) To: Chuan Liu via B4 Relay Cc: Neil Armstrong, Michael Turquette, Stephen Boyd, Kevin Hilman, Martin Blumenstingl, chuan.liu, linux-amlogic, linux-clk, linux-arm-kernel, linux-kernel On Fri 20 Sep 2024 at 16:13, Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@kernel.org> wrote: > From: Chuan Liu <chuan.liu@amlogic.com> > > 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. 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. 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 during > 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. The vast majority of those PLL should be RO then. If you can relock it, you should be able to re-init it as well. > > Signed-off-by: Chuan Liu <chuan.liu@amlogic.com> > --- > 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; I'm not OK with this. > > 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 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] clk: meson: pll: Update the meson_clk_pll_init execution judgment logic 2024-09-24 8:50 ` Jerome Brunet @ 2024-09-24 10:27 ` Chuan Liu 2024-09-24 13:35 ` Jerome Brunet 0 siblings, 1 reply; 5+ messages in thread From: Chuan Liu @ 2024-09-24 10:27 UTC (permalink / raw) To: Jerome Brunet, Chuan Liu via B4 Relay Cc: Neil Armstrong, Michael Turquette, Stephen Boyd, Kevin Hilman, Martin Blumenstingl, linux-amlogic, linux-clk, linux-arm-kernel, linux-kernel On 2024/9/24 16:50, Jerome Brunet wrote: > [ EXTERNAL EMAIL ] > > On Fri 20 Sep 2024 at 16:13, Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@kernel.org> wrote: > >> From: Chuan Liu <chuan.liu@amlogic.com> >> >> 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. > Apparently you messed something up with b4 ... 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 (to match the audio bit rate/audio and video synchronization, etc.). The 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. 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). 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 during >> 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. > The vast majority of those PLL should be RO then. > 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@amlogic.com> >> --- >> 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; > I'm not OK with this. > >> 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 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] clk: meson: pll: Update the meson_clk_pll_init execution judgment logic 2024-09-24 10:27 ` Chuan Liu @ 2024-09-24 13:35 ` Jerome Brunet 2024-09-25 6:51 ` Chuan Liu 0 siblings, 1 reply; 5+ messages in thread From: Jerome Brunet @ 2024-09-24 13:35 UTC (permalink / raw) To: Chuan Liu Cc: Chuan Liu via B4 Relay, Neil Armstrong, Michael Turquette, Stephen Boyd, Kevin Hilman, Martin Blumenstingl, linux-amlogic, linux-clk, linux-arm-kernel, linux-kernel On Tue 24 Sep 2024 at 18:27, Chuan Liu <chuan.liu@amlogic.com> wrote: > On 2024/9/24 16:50, Jerome Brunet wrote: >> [ EXTERNAL EMAIL ] >> >> On Fri 20 Sep 2024 at 16:13, Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@kernel.org> wrote: >> >>> From: Chuan Liu <chuan.liu@amlogic.com> >>> >>> 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. >> Apparently you messed something up with b4 ... > > > 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 Whatever was feeding the audio fifo is gone by that stage and fifo, tdm 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.). The > 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. HDMI, you presumably want to avoid a glitch in video until you're ready to reconfigure the pipeline. That would be a valid use-case for CLK_MESON_PLL_NOINIT_ENABLED. > > > 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). I understood your idea the first time around. I still do not agree. 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 during >>> 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. >> The vast majority of those PLL should be RO then. >> 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@amlogic.com> >>> --- >>> 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; >> I'm not OK with this. >> >>> 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 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] clk: meson: pll: Update the meson_clk_pll_init execution judgment logic 2024-09-24 13:35 ` Jerome Brunet @ 2024-09-25 6:51 ` Chuan Liu 0 siblings, 0 replies; 5+ messages in thread From: Chuan Liu @ 2024-09-25 6:51 UTC (permalink / raw) To: Jerome Brunet Cc: Chuan Liu via B4 Relay, Neil Armstrong, Michael Turquette, Stephen Boyd, Kevin Hilman, Martin Blumenstingl, linux-amlogic, linux-clk, linux-arm-kernel, linux-kernel On 2024/9/24 21:35, Jerome Brunet wrote: > [ EXTERNAL EMAIL ] > > On Tue 24 Sep 2024 at 18:27, Chuan Liu <chuan.liu@amlogic.com> wrote: > >> On 2024/9/24 16:50, Jerome Brunet wrote: >>> [ EXTERNAL EMAIL ] >>> >>> On Fri 20 Sep 2024 at 16:13, Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@kernel.org> wrote: >>> >>>> From: Chuan Liu <chuan.liu@amlogic.com> >>>> >>>> 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. >>> Apparently you messed something up with b4 ... >> >> 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 > Whatever was feeding the audio fifo is gone by that stage and fifo, tdm > 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. I asked a colleague of our audio driver, and he replied that there were the following scenarios: During the bootloader phase, our audio stream and video stream would be output together through HDMI, so hifi_pll also needed to ensure continuity during the kernel. Of course, it doesn't matter, our driver can meet this need. >> (to match the audio bit rate/audio and video synchronization, etc.). The >> 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. > HDMI, you presumably want to avoid a glitch in video until you're ready > to reconfigure the pipeline. That would be a valid use-case for > CLK_MESON_PLL_NOINIT_ENABLED. > >> >> 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). > I understood your idea the first time around. I still do not agree. > 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. The original intention of this patch is to simplify the PLL init execution logic, and the kernel is compatible with the bootloader initializing PLL and not initializing PLL, and can increase the fault tolerance of the driver. Although this treatment hides the coupling relationship with the bootloader, balancing the risks and benefits, I think the benefits are greater. This is just an optimization patch of low importance, if you think this patch is bad after weighing it, then ignore it. >> 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 during >>>> 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. >>> The vast majority of those PLL should be RO then. >>> 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@amlogic.com> >>>> --- >>>> 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; >>> I'm not OK with this. >>> >>>> 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 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-09-25 6:59 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-09-20 8:13 [PATCH] clk: meson: pll: Update the meson_clk_pll_init execution judgment logic Chuan Liu via B4 Relay 2024-09-24 8:50 ` Jerome Brunet 2024-09-24 10:27 ` Chuan Liu 2024-09-24 13:35 ` Jerome Brunet 2024-09-25 6:51 ` Chuan Liu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®