[PATCH] clk: imx: add CLK_GET_RATE_NOCACHE flag for i.MX8M composite clock

[PATCH] clk: imx: add CLK_GET_RATE_NOCACHE flag for i.MX8M composite clock

[Anson Huang]

On i.MX8M, some of the bus clocks' rate could be changed in TF-A,
so Linux clock framework does NOT know it at all, need to add
CLK_GET_RATE_NOCACHE flag for composite clock to make sure the
clk rate from Linux clock tree always matches the HW settings.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 drivers/clk/imx/clk-composite-8m.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
index 527ade1..d9147ec 100644
--- a/drivers/clk/imx/clk-composite-8m.c
+++ b/drivers/clk/imx/clk-composite-8m.c
@@ -161,6 +161,8 @@ struct clk *imx8m_clk_composite_flags(const char *name,
 	gate->reg = reg;
 	gate->bit_idx = PCG_CGC_SHIFT;
 
+	flags |= CLK_GET_RATE_NOCACHE;
+
 	hw = clk_hw_register_composite(NULL, name, parent_names, num_parents,
 			mux_hw, &clk_mux_ops, div_hw,
 			&imx8m_clk_composite_divider_ops,
-- 
2.7.4

Re: [PATCH] clk: imx: add CLK_GET_RATE_NOCACHE flag for i.MX8M composite clock

[Fabio Estevam]

Hi Anson,

On Tue, Dec 18, 2018 at 12:56 AM Anson Huang <anson.huang@nxp.com> wrote:
>
> On i.MX8M, some of the bus clocks' rate could be changed in TF-A,

Do you mean ATF (ARM Trusted Firmware) instead?

Re: [PATCH] clk: imx: add CLK_GET_RATE_NOCACHE flag for i.MX8M composite clock

[Lucas Stach]

Am Dienstag, den 18.12.2018, 08:24 -0200 schrieb Fabio Estevam:
> Hi Anson,
> 
> On Tue, Dec 18, 2018 at 12:56 AM Anson Huang <anson.huang@nxp.com>
> wrote:
> > 
> > On i.MX8M, some of the bus clocks' rate could be changed in TF-A,
> 
> Do you mean ATF (ARM Trusted Firmware) instead?

TF-A is the name of the day for what was formerly known as ATF...

However I don't think that it's correct to just don't cache the clock
settings. Normally the secure world firmware should not change any
clock settings at runtime, or it would run into all kinds of conflicts
with the clock driver. So there are probably some well known points in
time like a suspend or resume event when the firmware might change
clock settings, so we could instead use those to trigger an explicit
invalidate of the clock caches with much lower overhead.

Regards,
Lucas

Re: [PATCH] clk: imx: add CLK_GET_RATE_NOCACHE flag for i.MX8M composite clock

[Anson Huang]

Hi, Lucas

From Anson's iPhone 6


> 在 2018年12月18日，18:40，Lucas Stach <l.stach@pengutronix.de> 写道：
> 
> Am Dienstag, den 18.12.2018, 08:24 -0200 schrieb Fabio Estevam:
>> Hi Anson,
>> 
>> On Tue, Dec 18, 2018 at 12:56 AM Anson Huang <anson.huang@nxp.com>
>> wrote:
>>> 
>>> On i.MX8M, some of the bus clocks' rate could be changed in TF-A,
>> 
>> Do you mean ATF (ARM Trusted Firmware) instead?
> 
> TF-A is the name of the day for what was formerly known as ATF...
> 
> However I don't think that it's correct to just don't cache the clock
> settings. Normally the secure world firmware should not change any
> clock settings at runtime, or it would run into all kinds of conflicts
> with the clock driver. So there are probably some well known points in
> time like a suspend or resume event when the firmware might change
> clock settings, so we could instead use those to trigger an explicit
> invalidate of the clock caches with much lower overhead.
> 
> Regards,
> Lucas

There is bus-freq feature on imx8m which is to scale ddr clock, this is done in ARM Trusted Firmware, for some setpoints, the DDR PLL clock rate must be changed directly in TF-A, but its child clock like dram core is unaware in Linux kernel, so the clock rate will mismatch with hardware, since ddr related clocks will NOT used by any module in Linux kernel, so it will NOT introduce any conflict.

Regarding about the over head, yes, the change in common composite clock register has too many over head for other clocks, what if I ONLY have dram core clock to pass the CLK_GET_RATE_NOCACHE flag to register the composite clock?

Anson.

Re: [PATCH] clk: imx: add CLK_GET_RATE_NOCACHE flag for i.MX8M composite clock

[Lucas Stach]

Hi Anson,

Am Dienstag, den 18.12.2018, 13:35 +0000 schrieb Anson Huang:
> Hi, Lucas
> 
> From Anson's iPhone 6
> 
> 
> > 在 2018年12月18日，18:40，Lucas Stach <l.stach@pengutronix.de> 写道：
> > 
> > Am Dienstag, den 18.12.2018, 08:24 -0200 schrieb Fabio Estevam:
> > > Hi Anson,
> > > 
> > > On Tue, Dec 18, 2018 at 12:56 AM Anson Huang <anson.huang@nxp.com
> > > >
> > > wrote:
> > > > 
> > > > On i.MX8M, some of the bus clocks' rate could be changed in TF-
> > > > A,
> > > 
> > > Do you mean ATF (ARM Trusted Firmware) instead?
> > 
> > TF-A is the name of the day for what was formerly known as ATF...
> > 
> > However I don't think that it's correct to just don't cache the
> > clock
> > settings. Normally the secure world firmware should not change any
> > clock settings at runtime, or it would run into all kinds of
> > conflicts
> > with the clock driver. So there are probably some well known points
> > in
> > time like a suspend or resume event when the firmware might change
> > clock settings, so we could instead use those to trigger an
> > explicit
> > invalidate of the clock caches with much lower overhead.
> > 
> > Regards,
> > Lucas
> 
> There is bus-freq feature on imx8m which is to scale ddr clock, this
> is done in ARM Trusted Firmware, for some setpoints, the DDR PLL
> clock rate must be changed directly in TF-A, but its child clock like
> dram core is unaware in Linux kernel, so the clock rate will mismatch
> with hardware, since ddr related clocks will NOT used by any module
> in Linux kernel, so it will NOT introduce any conflict.

I don't think there is anything implementing the bus frequency scaling
in mainline, right?

> Regarding about the over head, yes, the change in common composite
> clock register has too many over head for other clocks, what if I
> ONLY have dram core clock to pass the CLK_GET_RATE_NOCACHE flag to
> register the composite clock?

IMHO marking clocks under TF-A control explicitly as nocache would be
much more acceptable than doing it for every composite clock. This
seems okay for a short term solution.

Still I think that whatever is causing the bus frequency scale to
change should have a way to explicitly invalidate the clock cache for
the affected clocks eventually.

Regards,
Lucas

Re: [PATCH] clk: imx: add CLK_GET_RATE_NOCACHE flag for i.MX8M composite clock

[Anson Huang]

Hi, Lucas

From Anson's iPhone 6


> 在 2018年12月18日，21:41，Lucas Stach <l.stach@pengutronix.de> 写道：
> 
> Hi Anson,
> 
> Am Dienstag, den 18.12.2018, 13:35 +0000 schrieb Anson Huang:
>> Hi, Lucas
>> 
>> From Anson's iPhone 6
>> 
>> 
>>> 在 2018年12月18日，18:40，Lucas Stach <l.stach@pengutronix.de> 写道：
>>> 
>>> Am Dienstag, den 18.12.2018, 08:24 -0200 schrieb Fabio Estevam:
>>>> Hi Anson,
>>>> 
>>>> On Tue, Dec 18, 2018 at 12:56 AM Anson Huang <anson.huang@nxp.com
>>>>> 
>>>> wrote:
>>>>> 
>>>>> On i.MX8M, some of the bus clocks' rate could be changed in TF-
>>>>> A,
>>>> 
>>>> Do you mean ATF (ARM Trusted Firmware) instead?
>>> 
>>> TF-A is the name of the day for what was formerly known as ATF...
>>> 
>>> However I don't think that it's correct to just don't cache the
>>> clock
>>> settings. Normally the secure world firmware should not change any
>>> clock settings at runtime, or it would run into all kinds of
>>> conflicts
>>> with the clock driver. So there are probably some well known points
>>> in
>>> time like a suspend or resume event when the firmware might change
>>> clock settings, so we could instead use those to trigger an
>>> explicit
>>> invalidate of the clock caches with much lower overhead.
>>> 
>>> Regards,
>>> Lucas
>> 
>> There is bus-freq feature on imx8m which is to scale ddr clock, this
>> is done in ARM Trusted Firmware, for some setpoints, the DDR PLL
>> clock rate must be changed directly in TF-A, but its child clock like
>> dram core is unaware in Linux kernel, so the clock rate will mismatch
>> with hardware, since ddr related clocks will NOT used by any module
>> in Linux kernel, so it will NOT introduce any conflict.
> 
> I don't think there is anything implementing the bus frequency scaling
> in mainline, right?

Yes, mainline has no bus-freq scaling so far, but internally we use same composite clock driver as mainline, and bus-freq clock rate issue/bug reported during our internal test, that is why I create this patch to easy our next kernel upgrade.

> 
>> Regarding about the over head, yes, the change in common composite
>> clock register has too many over head for other clocks, what if I
>> ONLY have dram core clock to pass the CLK_GET_RATE_NOCACHE flag to
>> register the composite clock?
> 
> IMHO marking clocks under TF-A control explicitly as nocache would be
> much more acceptable than doing it for every composite clock. This
> seems okay for a short term solution.
> 
> Still I think that whatever is causing the bus frequency scale to
> change should have a way to explicitly invalidate the clock cache for
> the affected clocks eventually.

It is because the DDR PLL/clocks can only be changed with strict DDR freq change flow, and it is done in TF-A, Linux kernel can NOT touch it in runtime, so we have to mark the child clock of DDR PLL to be uncached, in V2 patch, I will just add the flag for the DDR PLL child clocks to be a shorten solution, should be only very few ones, hope it is acceptable, thanks.

Anson.


> 
> Regards,
> Lucas

Re: [PATCH] clk: imx: add CLK_GET_RATE_NOCACHE flag for i.MX8M composite clock

[Lucas Stach]

Am Dienstag, den 18.12.2018, 13:53 +0000 schrieb Anson Huang:
[...]
> > 
> > > Regarding about the over head, yes, the change in common composite
> > > clock register has too many over head for other clocks, what if I
> > > ONLY have dram core clock to pass the CLK_GET_RATE_NOCACHE flag to
> > > register the composite clock?
> > 
> > IMHO marking clocks under TF-A control explicitly as nocache would be
> > much more acceptable than doing it for every composite clock. This
> > seems okay for a short term solution.
> > 
> > Still I think that whatever is causing the bus frequency scale to
> > change should have a way to explicitly invalidate the clock cache for
> > the affected clocks eventually.
> 
> It is because the DDR PLL/clocks can only be changed with strict DDR
> freq change flow, and it is done in TF-A, Linux kernel can NOT touch
> it in runtime, so we have to mark the child clock of DDR PLL to be
> uncached, in V2 patch, I will just add the flag for the DDR PLL child
> clocks to be a shorten solution, should be only very few ones, hope
> it is acceptable, thanks.

I fully understand why you are doing the frequency change in TF-A and I
agree with the reasoning to do so. I also think that using uncached for
the few clocks under TF-A control is fine for now.

But if/when the bus frequency scaling is actually implemented for
upstream I think the flow should look something like that:

1. Bus freq scaling driver determines that a change is necessary
2. Scaling driver calls into TF-A to do the change
3. TF-A reconfigures clock rates
4. Scaling driver calls into clock driver to signal that a clock change
might have happened
5. Clock driver invalidates and recalculates cached values for the
affected clocks

Regards,
Lucas

Re: [PATCH] clk: imx: add CLK_GET_RATE_NOCACHE flag for i.MX8M composite clock

[Stephen Boyd]

Quoting Lucas Stach (2018-12-18 06:02:28)
> Am Dienstag, den 18.12.2018, 13:53 +0000 schrieb Anson Huang:
> [...]
> > > 
> > > > Regarding about the over head, yes, the change in common composite
> > > > clock register has too many over head for other clocks, what if I
> > > > ONLY have dram core clock to pass the CLK_GET_RATE_NOCACHE flag to
> > > > register the composite clock?
> > > 
> > > IMHO marking clocks under TF-A control explicitly as nocache would be
> > > much more acceptable than doing it for every composite clock. This
> > > seems okay for a short term solution.
> > > 
> > > Still I think that whatever is causing the bus frequency scale to
> > > change should have a way to explicitly invalidate the clock cache for
> > > the affected clocks eventually.
> > 
> > It is because the DDR PLL/clocks can only be changed with strict DDR
> > freq change flow, and it is done in TF-A, Linux kernel can NOT touch
> > it in runtime, so we have to mark the child clock of DDR PLL to be
> > uncached, in V2 patch, I will just add the flag for the DDR PLL child
> > clocks to be a shorten solution, should be only very few ones, hope
> > it is acceptable, thanks.
> 
> I fully understand why you are doing the frequency change in TF-A and I
> agree with the reasoning to do so. I also think that using uncached for
> the few clocks under TF-A control is fine for now.
> 
> But if/when the bus frequency scaling is actually implemented for
> upstream I think the flow should look something like that:
> 
> 1. Bus freq scaling driver determines that a change is necessary
> 2. Scaling driver calls into TF-A to do the change
> 3. TF-A reconfigures clock rates
> 4. Scaling driver calls into clock driver to signal that a clock change
> might have happened
> 5. Clock driver invalidates and recalculates cached values for the
> affected clocks
> 

Does any clk consuming driver of the downstream clks that are branched
off of the bus clk managed by firmware care about the frequency? Or do
they just want the clk to be on. If they don't care then it's possible
to break the parent dependency and just not care to tell them what the
bus frequency is anymore.

I don't know how you would implement #4 above, besides by having the bus
freq scaling driver use clk_set_rate() to tell the bus clk that it wants
a new rate and then having that clk implementation do #2. That way the
rate propagation works without having to notify clk code somehow.

RE: [PATCH] clk: imx: add CLK_GET_RATE_NOCACHE flag for i.MX8M composite clock

[Anson Huang]

Hi, Stephen

Best Regards!
Anson Huang

> -----Original Message-----
> From: Stephen Boyd [mailto:sboyd@kernel.org]
> Sent: 2018年12月19日 3:32
> To: Anson Huang <anson.huang@nxp.com>; Lucas Stach
> <l.stach@pengutronix.de>
> Cc: Fabio Estevam <festevam@gmail.com>; s.hauer@pengutronix.de;
> linux-kernel@vger.kernel.org; linux-clk@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam
> <fabio.estevam@nxp.com>; shawnguo@kernel.org;
> mturquette@baylibre.com; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH] clk: imx: add CLK_GET_RATE_NOCACHE flag for i.MX8M
> composite clock
> 
> Quoting Lucas Stach (2018-12-18 06:02:28)
> > Am Dienstag, den 18.12.2018, 13:53 +0000 schrieb Anson Huang:
> > [...]
> > > >
> > > > > Regarding about the over head, yes, the change in common
> > > > > composite clock register has too many over head for other
> > > > > clocks, what if I ONLY have dram core clock to pass the
> > > > > CLK_GET_RATE_NOCACHE flag to register the composite clock?
> > > >
> > > > IMHO marking clocks under TF-A control explicitly as nocache would
> > > > be much more acceptable than doing it for every composite clock.
> > > > This seems okay for a short term solution.
> > > >
> > > > Still I think that whatever is causing the bus frequency scale to
> > > > change should have a way to explicitly invalidate the clock cache
> > > > for the affected clocks eventually.
> > >
> > > It is because the DDR PLL/clocks can only be changed with strict DDR
> > > freq change flow, and it is done in TF-A, Linux kernel can NOT touch
> > > it in runtime, so we have to mark the child clock of DDR PLL to be
> > > uncached, in V2 patch, I will just add the flag for the DDR PLL
> > > child clocks to be a shorten solution, should be only very few ones,
> > > hope it is acceptable, thanks.
> >
> > I fully understand why you are doing the frequency change in TF-A and
> > I agree with the reasoning to do so. I also think that using uncached
> > for the few clocks under TF-A control is fine for now.
> >
> > But if/when the bus frequency scaling is actually implemented for
> > upstream I think the flow should look something like that:
> >
> > 1. Bus freq scaling driver determines that a change is necessary 2.
> > Scaling driver calls into TF-A to do the change 3. TF-A reconfigures
> > clock rates 4. Scaling driver calls into clock driver to signal that a
> > clock change might have happened 5. Clock driver invalidates and
> > recalculates cached values for the affected clocks
> >
> 
> Does any clk consuming driver of the downstream clks that are branched off of
> the bus clk managed by firmware care about the frequency? Or do they just
> want the clk to be on. If they don't care then it's possible to break the parent
> dependency and just not care to tell them what the bus frequency is anymore.
> 
> I don't know how you would implement #4 above, besides by having the bus
> freq scaling driver use clk_set_rate() to tell the bus clk that it wants a new rate
> and then having that clk implementation do #2. That way the rate propagation
> works without having to notify clk code somehow.

In our case, the original clock relationship is as below, when DDR freq needs to be scaled, below things are proceeded:

Clock tree:
IMX8MQ_DRAM_PLL2
	IMX8MQ_DRAM_PLL2_DIV
		IMX8MQ_DRAM_PLL2_OUT
			IMX8MQ_DRAM_PLL_OUT
				IMX8MQ_CLK_DRAM_CORE

1. Linux kernel do SMC call into TF-A;
2. in TF-A, we have to scale the IMX8MQ_DRAM_PLL2 to dedicated frequency along with DDR freq scale flow;
3. After TF-A done the DDR freq scale and return back to Linux, all the downstream of IMX8MQ_DRAM_PLL2 clock will
  have mismatch clock rate between clock tree and HW settings since they are cached;
4. Maybe we can call clk_set_rate in Linux for IMX8MQ_DRAM_PLL2 again (although the HW settings are already expected)
  to update the downstream clocks, looks like can fix it, but will the call be skipped by clock framework when clock driver
  re-calculate the PLL rate based on HW setting, as HW setting already equal the rate wants to be set, and clk_propagate_rate_change
  will be skipped too or it will automatically update all child clocks' rate?

Is it correct? if all child clocks' rate can be updated by clk_propagate_rate_change(), then we no need to add any clock flag,
just make sure after TF-A finish the DDR freq scale, make sure calling the clk_set_rate() for the IMX8MQ_DRAM_PLL2, then everything should be correct?

Anson.