| Message ID | 20201231142149.26062-1-martin.kepplinger@puri.sm (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Revert "clk: imx: fix composite peripheral flags" | expand |
Hi Martin, On Thu, Dec 31, 2020 at 11:22 AM Martin Kepplinger <martin.kepplinger@puri.sm> wrote: > > This reverts commit 936c383673b9e3007432f17140ac62de53d87db9. > > It breaks clock reparenting via devfreq on the imx8mq used in the > Librem 5 phone. When switching dram frequency (which worked before) > the system now hangs after this where the dram_apb clock cannot be > set: > > [ 129.391755] imx8m-ddrc-devfreq 3d400000.memory-controller: failed to > set dram_apb parent: -16 > [ 129.391959] imx8m-ddrc-devfreq 3d400000.memory-controller: ddrc > failed freq switch to 25000000 from 800000000: error -16. now at 25000000 > [ 129.406133] imx8m-ddrc-devfreq 3d400000.memory-controller: failed to > update frequency from PM QoS (-16) I am wondering whether IMX8MQ_CLK_DRAM_ALT should also be marked as CLK_IS_CRITICAL. Could you please try the following change without the revert? --- a/drivers/clk/imx/clk-imx8mq.c +++ b/drivers/clk/imx/clk-imx8mq.c @@ -458,7 +458,7 @@ static int imx8mq_clocks_probe(struct platform_device *pdev) * Mark with GET_RATE_NOCACHE to always read div value from hardware */ hws[IMX8MQ_CLK_DRAM_CORE] = imx_clk_hw_mux2_flags("dram_core_clk", base + 0x9800, 24, 1, imx8mq_dram_core_sels, ARRAY_SIZE(imx8mq_dram_core_sels), CLK_IS_CRITICAL); - hws[IMX8MQ_CLK_DRAM_ALT] = __imx8m_clk_hw_composite("dram_alt", imx8mq_dram_alt_sels, base + 0xa000, CLK_GET_RATE_NOCACHE); + hws[IMX8MQ_CLK_DRAM_ALT] = __imx8m_clk_hw_composite("dram_alt", imx8mq_dram_alt_sels, base + 0xa000, CLK_IS_CRITICAL | CLK_GET_RATE_NOCACHE); hws[IMX8MQ_CLK_DRAM_APB] = __imx8m_clk_hw_composite("dram_apb", imx8mq_dram_apb_sels, base + 0xa080, CLK_IS_CRITICAL | CLK_GET_RATE_NOCACHE); Thanks
On 20-12-31 17:33:40, Fabio Estevam wrote: > Hi Martin, > > On Thu, Dec 31, 2020 at 11:22 AM Martin Kepplinger > <martin.kepplinger@puri.sm> wrote: > > > > This reverts commit 936c383673b9e3007432f17140ac62de53d87db9. > > > > It breaks clock reparenting via devfreq on the imx8mq used in the > > Librem 5 phone. When switching dram frequency (which worked before) > > the system now hangs after this where the dram_apb clock cannot be > > set: > > > > [ 129.391755] imx8m-ddrc-devfreq 3d400000.memory-controller: failed to > > set dram_apb parent: -16 > > [ 129.391959] imx8m-ddrc-devfreq 3d400000.memory-controller: ddrc > > failed freq switch to 25000000 from 800000000: error -16. now at 25000000 > > [ 129.406133] imx8m-ddrc-devfreq 3d400000.memory-controller: failed to > > update frequency from PM QoS (-16) > > I am wondering whether IMX8MQ_CLK_DRAM_ALT should also be marked as > CLK_IS_CRITICAL. > Hmm, the way the DRAM clocks are right registered right now is a real mess. The DRAM clocks on i.MX8M are changed in TF-A, but the kernel still needs to register them to keep track of the clock tree. Martin, I already have a patchset waiting to be shipped which doesn't only fix the 8MQ, but all the 8M platforms. Unfortunately, I haven't had the time to work on that in the last couple of weeks but I intend to switch back to it soon. Fabio, marking the DRAM clocks as critical will not allow the set_parent to be done, as CLK_IS_CRITICAL flag and set_parent do not go together. As of now the devfreq tries to reparent to be consistent with TF-A configuration. My approach here was to make the DRAM clocks read-only. This means adding some stuff in the clock core subsystem too. > Could you please try the following change without the revert? > > --- a/drivers/clk/imx/clk-imx8mq.c > +++ b/drivers/clk/imx/clk-imx8mq.c > @@ -458,7 +458,7 @@ static int imx8mq_clocks_probe(struct platform_device *pdev) > * Mark with GET_RATE_NOCACHE to always read div value from hardware > */ > hws[IMX8MQ_CLK_DRAM_CORE] = > imx_clk_hw_mux2_flags("dram_core_clk", base + 0x9800, 24, 1, > imx8mq_dram_core_sels, ARRAY_SIZE(imx8mq_dram_core_sels), > CLK_IS_CRITICAL); > - hws[IMX8MQ_CLK_DRAM_ALT] = > __imx8m_clk_hw_composite("dram_alt", imx8mq_dram_alt_sels, base + > 0xa000, CLK_GET_RATE_NOCACHE); > + hws[IMX8MQ_CLK_DRAM_ALT] = > __imx8m_clk_hw_composite("dram_alt", imx8mq_dram_alt_sels, base + > 0xa000, CLK_IS_CRITICAL | CLK_GET_RATE_NOCACHE); > hws[IMX8MQ_CLK_DRAM_APB] = > __imx8m_clk_hw_composite("dram_apb", imx8mq_dram_apb_sels, base + > 0xa080, CLK_IS_CRITICAL | CLK_GET_RATE_NOCACHE); > > Thanks
On 04.01.21 13:05, Abel Vesa wrote: > On 20-12-31 17:33:40, Fabio Estevam wrote: >> Hi Martin, >> >> On Thu, Dec 31, 2020 at 11:22 AM Martin Kepplinger >> <martin.kepplinger@puri.sm> wrote: >>> >>> This reverts commit 936c383673b9e3007432f17140ac62de53d87db9. >>> >>> It breaks clock reparenting via devfreq on the imx8mq used in the >>> Librem 5 phone. When switching dram frequency (which worked before) >>> the system now hangs after this where the dram_apb clock cannot be >>> set: >>> >>> [ 129.391755] imx8m-ddrc-devfreq 3d400000.memory-controller: failed to >>> set dram_apb parent: -16 >>> [ 129.391959] imx8m-ddrc-devfreq 3d400000.memory-controller: ddrc >>> failed freq switch to 25000000 from 800000000: error -16. now at 25000000 >>> [ 129.406133] imx8m-ddrc-devfreq 3d400000.memory-controller: failed to >>> update frequency from PM QoS (-16) >> >> I am wondering whether IMX8MQ_CLK_DRAM_ALT should also be marked as >> CLK_IS_CRITICAL. >> > > Hmm, the way the DRAM clocks are right registered right now is a real mess. > The DRAM clocks on i.MX8M are changed in TF-A, but the kernel still needs to > register them to keep track of the clock tree. > > Martin, I already have a patchset waiting to be shipped which doesn't > only fix the 8MQ, but all the 8M platforms. Unfortunately, I haven't had the time > to work on that in the last couple of weeks but I intend to switch back to it soon. > > Fabio, marking the DRAM clocks as critical will not allow the set_parent to be done, > as CLK_IS_CRITICAL flag and set_parent do not go together. As of now the devfreq > tries to reparent to be consistent with TF-A configuration. > > My approach here was to make the DRAM clocks read-only. This means adding some > stuff in the clock core subsystem too. Hi Abel, thanks a lot for the update. I'm looking forward to seeing your patchset. martin
diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c index 2c309e3dc8e3..78fb7e52a42a 100644 --- a/drivers/clk/imx/clk-composite-8m.c +++ b/drivers/clk/imx/clk-composite-8m.c @@ -216,7 +216,6 @@ struct clk_hw *imx8m_clk_hw_composite_flags(const char *name, div->width = PCG_PREDIV_WIDTH; divider_ops = &imx8m_clk_composite_divider_ops; mux_ops = &clk_mux_ops; - flags |= CLK_SET_PARENT_GATE; } div->lock = &imx_ccm_lock;
This reverts commit 936c383673b9e3007432f17140ac62de53d87db9. It breaks clock reparenting via devfreq on the imx8mq used in the Librem 5 phone. When switching dram frequency (which worked before) the system now hangs after this where the dram_apb clock cannot be set: [ 129.391755] imx8m-ddrc-devfreq 3d400000.memory-controller: failed to set dram_apb parent: -16 [ 129.391959] imx8m-ddrc-devfreq 3d400000.memory-controller: ddrc failed freq switch to 25000000 from 800000000: error -16. now at 25000000 [ 129.406133] imx8m-ddrc-devfreq 3d400000.memory-controller: failed to update frequency from PM QoS (-16) Note that on the Librem 5 devkit that uses a different revision of the imx8mq SoC, the added flag does *not* break said clock reparenting so there might be subtle differences here. Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm> --- drivers/clk/imx/clk-composite-8m.c | 1 - 1 file changed, 1 deletion(-)