Re: Clocks used by another OS/CPU (was: Re: [RFC PATCH] clk: renesas: cpg-mssr: Add interface for critical core clocks)

[Peter De Schrijver]

On Mon, Jul 03, 2017 at 10:17:22AM +0100, Sudeep Holla wrote:
> 
> 
> On 01/07/17 19:14, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Sat, Jul 01, 2017 at 07:02:48AM +0200, Dirk Behme wrote:
> 
> [...]
> 
> >>
> >>
> >> The other problem is security related. If, at all, you have to do it the
> >> other way around, then:
> >>
> >> Make Linux a consumer of the other CPU's (trusted/trustzone/whatever
> >> secured) OS clock driver.
> 
> Yes, that's better and is getting common on newer platforms. They have
> separate M-class(or even low A-class e.g. A5/A7) processors to handle
> all the system management.
> 
> The new ARM SCMI specification[0][1] is designed to standardize the
> interface. It covers the clocks in clock protocol.
> 

Yes, however this doesn't exist on older SoCs which still have multiple CPU's

Peter.

Re: Clocks used by another OS/CPU (was: Re: [RFC PATCH] clk: renesas: cpg-mssr: Add interface for critical core clocks)

[Peter De Schrijver]

On Thu, Jun 29, 2017 at 11:27:28AM +0200, Geert Uytterhoeven wrote:
> Hi Dirk,
> 
> CC clock, ARM, DT, PM people
> 
> TL;DR: Clocks may be in use by another CPU not running Linux, while Linux
> disables them as being unused.
> 

There is that but also Linux should not be allowed to change the rate and
parent. Otherwise your R7 sw will likely fail as well. I think it makes sense
to have some DT property which informs linux which clocks it should not touch.
At least assuming clock control isn't moved to a separate coprocessor. In that
case any policy can ofcourse be implemented in the coprocessor.

Peter.


> On Mon, Jun 26, 2017 at 1:30 PM, Dirk Behme  wrote:
> > With commit 72f5df2c2bbb6 ("clk: renesas: cpg-mssr: Migrate to
> > CLK_IS_CRITICAL") we are able to handle critical module clocks.
> > Introduce the same logic for critical core clocks.
> >
> > Signed-off-by: Dirk Behme 
> > ---
> > Commit
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/clk/renesas?id=72f5df2c2bbb66d4a555cb51eb9f412abf1af77f
> >
> > is quite nice to avoid *module* clocks being disabled. Unfortunately,
> > there are *core* clocks, too. E.g. using an other OS on the Cortex R7
> > core of the r8a7795, the 'canfd' is a quite popular core clock which
> > shouldn't be disabled by Linux.
> >
> > Therefore, this patch is a proposal to use the same 'mark clocks as
> > critical' logic implemented for the module clocks for the core
> > clocks, too.
> >
> > Opinions?
> 
> On r8a7795, there are several Cortex A cores running Linux, and a Cortex R7
> core which may run another OS.
> This is an interesting issue, and relevant to other SoCs, too.
> 
> In this particular case, the "canfd" clock is a core clock used as an
> auxiliary clock for the CAN0, CAN1, and CANFD interfaces.  This can lead
> to three scenarios:
>   1. Linux controls all CAN interfaces
>  => no issue,
>   2. The OS on the RT CPU controls all CAN interfaces
>  => issue, Linux disables the clock
>   3. Mix of 1 and 2
>  => More issues.
> Of course this is not limited to clocks, but also to e.g. PM domains.
> 
> How can this be handled?
> I believe just marking the "canfd" clock critical is not the right solution,
> as about any clock could be used by the RT CPU.
> 
> Still, Linux needs to be made aware that devices (clocks and PM domains) are
> controlled by another CPU/OS.
> 
> Should this be described in DT? It feels like software policy to me.
> 
> Note that we (mainline) currently don't describe the Cortex R7 core in DT.
> Dirk: do you describe it?
> 
> Summary:
>   1. Core/module clocks are described in the clock driver (not in DT),
>   2. Unused clocks are disabled by CCF,
>   3. Clocks may be in use by the Real-Time CPU core, running another OS,
>   4. How to communicate to Linux which clocks are under control of the RT CPU?
> 
> Thanks for your comments!
> 
> >  drivers/clk/renesas/clk-div6.c | 17 +++--
> >  drivers/clk/renesas/clk-div6.h |  4 +++-
> >  drivers/clk/renesas/r8a7795-cpg-mssr.c |  7 +++
> >  drivers/clk/renesas/renesas-cpg-mssr.c |  3 ++-
> >  drivers/clk/renesas/renesas-cpg-mssr.h |  8 
> >  5 files changed, 35 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/clk/renesas/clk-div6.c b/drivers/clk/renesas/clk-div6.c
> > index 0627860..5917e05 100644
> > --- a/drivers/clk/renesas/clk-div6.c
> > +++ b/drivers/clk/renesas/clk-div6.c
> > @@ -18,6 +18,7 @@
> >  #include 
> >  #include 
> >
> > +#include "renesas-cpg-mssr.h"
> >  #include "clk-div6.h"
> >
> >  #define CPG_DIV6_CKSTP BIT(8)
> > @@ -184,7 +185,9 @@ static const struct clk_ops cpg_div6_clock_ops = {
> >  struct clk * __init cpg_div6_register(const char *name,
> >   unsigned int num_parents,
> >   const char **parent_names,
> > - void __iomem *reg)
> > + void __iomem *reg,
> > + const struct cpg_mssr_info *info,
> > + unsigned int id)
> >  {
> > unsigned int valid_parents;
> > struct clk_init_data init;
> > @@ -246,6 +249,15 @@ struct clk * __init cpg_div6_register(const char *name,
> > init.name = name;
> > init.ops = _div6_clock_ops;
> > init.flags = CLK_IS_BASIC;
> > +   if (info) {
> > +   for (i = 0; i < info->num_crit_core_clks; i++)
> > +   if (id == info->crit_core_clks[i]) {
> > +   pr_devel("DIV6 %s setting 
> > CLK_IS_CRITICAL\n",
> > +name);
> > +   init.flags |= CLK_IS_CRITICAL;
> > +   break;
> > +   }
> > +   }
> > init.parent_names = parent_names;
> > init.num_parents = 

Re: [PATCH] clk: renesas: rcar-gen3: Add Z clock divider support

[Peter De Schrijver]

Hi Geert,

On Tue, Apr 25, 2017 at 12:28:29PM +0200, Geert Uytterhoeven wrote:
> Hi Peter,
> 
> On Tue, Apr 25, 2017 at 12:18 PM, Peter De Schrijver
> <pdeschrij...@nvidia.com> wrote:
> > On Mon, Apr 24, 2017 at 10:09:10AM +0200, Geert Uytterhoeven wrote:
> >> On Mon, Apr 24, 2017 at 10:03 AM, Peter De Schrijver
> >> <pdeschrij...@nvidia.com> wrote:
> >> >> > --- a/drivers/clk/renesas/rcar-gen3-cpg.c
> >> >> > +++ b/drivers/clk/renesas/rcar-gen3-cpg.c
> >> >>
> >> >> > +static unsigned long cpg_z_clk_recalc_rate(struct clk_hw *hw,
> >> >> > +  unsigned long parent_rate)
> >> >> > +{
> >> >> > +   struct cpg_z_clk *zclk = to_z_clk(hw);
> >> >> > +   unsigned int mult;
> >> >> > +   unsigned int val;
> >> >> > +   unsigned long rate;
> >> >> > +
> >> >> > +   val = (clk_readl(zclk->reg) & CPG_FRQCRC_ZFC_MASK)
> >> >> > +   >> CPG_FRQCRC_ZFC_SHIFT;
> >> >> > +   mult = 32 - val;
> >> >> > +
> >> >> > +   rate = div_u64((u64)parent_rate * mult + 16, 32);
> >> >> > +   /* Round to closest value at 100MHz unit */
> >> >> > +   rate = 1*DIV_ROUND_CLOSEST(rate, 1);
> >> >>
> >> >> Mike, Stephen: what's your opinion about such rounding tricks?
> >> >
> >> > Is this an actual divider or a pulse skipper?
> >>
> >> Forgive my ignorance, but what is the difference?
> >
> > A pulse skipper, as the name says, skips pulses. Eg, if you configure the
> > skipper to 3/4, it will skip 1 input clock pulse out of 4. The pulse width
> > will be the same as the parent clock pulse width though. A divider stretches
> > the clock pulse. So the output of a divider configured to 1/2 will have a
> > pulse width which is twice as large as the pulse width of the input clock.
> 
> OK, thanks! So a pulse skipper changes the duty cycle of the signal.
> 

Yes, it does.

> I would expect this to be a real divider, as that would allow the Cortex A53
> core to do work on both edges of the clock signal.

Maybe. Although I know we've run A53s from skippers. One difference is that a
real divider allows you to lower the voltage, while a skipper does not,
because it only changes the duty cycle as you mentioned.

> It's a bit difficult to measure, though ;-)
> 

Maybe there's a way to expose the clock signal on an external pin? Or you can
use a full chip emulator :)

Cheers,

Peter.

Re: [PATCH] clk: renesas: rcar-gen3: Add Z clock divider support

[Peter De Schrijver]

Hi Geert,

On Mon, Apr 24, 2017 at 10:09:10AM +0200, Geert Uytterhoeven wrote:
> Hi Peter,
> 
> On Mon, Apr 24, 2017 at 10:03 AM, Peter De Schrijver
> <pdeschrij...@nvidia.com> wrote:
> >> > --- a/drivers/clk/renesas/rcar-gen3-cpg.c
> >> > +++ b/drivers/clk/renesas/rcar-gen3-cpg.c
> >>
> >> > +static unsigned long cpg_z_clk_recalc_rate(struct clk_hw *hw,
> >> > +  unsigned long parent_rate)
> >> > +{
> >> > +   struct cpg_z_clk *zclk = to_z_clk(hw);
> >> > +   unsigned int mult;
> >> > +   unsigned int val;
> >> > +   unsigned long rate;
> >> > +
> >> > +   val = (clk_readl(zclk->reg) & CPG_FRQCRC_ZFC_MASK)
> >> > +   >> CPG_FRQCRC_ZFC_SHIFT;
> >> > +   mult = 32 - val;
> >> > +
> >> > +   rate = div_u64((u64)parent_rate * mult + 16, 32);
> >> > +   /* Round to closest value at 100MHz unit */
> >> > +   rate = 1*DIV_ROUND_CLOSEST(rate, 1);
> >>
> >> Mike, Stephen: what's your opinion about such rounding tricks?
> >
> > Is this an actual divider or a pulse skipper?
> 
> Forgive my ignorance, but what is the difference?

A pulse skipper, as the name says, skips pulses. Eg, if you configure the
skipper to 3/4, it will skip 1 input clock pulse out of 4. The pulse width
will be the same as the parent clock pulse width though. A divider stretches
the clock pulse. So the output of a divider configured to 1/2 will have a
pulse width which is twice as large as the pulse width of the input clock.

Cheers,

Peter.

Re: [PATCH] clk: renesas: rcar-gen3: Add Z clock divider support

[Peter De Schrijver]

On Fri, Apr 21, 2017 at 03:59:10PM +0200, Geert Uytterhoeven wrote:
> Hi Kaneko-san, and Mike, Stephen (see below),
> 
> On Wed, Apr 19, 2017 at 7:46 PM, Yoshihiro Kaneko  
> wrote:
> > From: Takeshi Kihara 
> >
> > This patch adds Z clock divider support for R-Car Gen3 SoC.
> >
> > Signed-off-by: Takeshi Kihara 
> > Signed-off-by: Yoshihiro Kaneko 
> 
> I gave this patch a try on Salvator-X.  On both R-Car H3 and M3-W,
> /sys/kernel/debug/clk/clk_summary reports:
> 
>.pll0  00  299880
>0 0
>   z   00  30
>0 0
> 
> The Z clock runs at 1.5 GHz, so the numbers are off by a factor of two.
> It seems the PLL post-divider of 1/2 is not taken into account.
> 
> > --- a/drivers/clk/renesas/rcar-gen3-cpg.c
> > +++ b/drivers/clk/renesas/rcar-gen3-cpg.c
> 
> > +static unsigned long cpg_z_clk_recalc_rate(struct clk_hw *hw,
> > +  unsigned long parent_rate)
> > +{
> > +   struct cpg_z_clk *zclk = to_z_clk(hw);
> > +   unsigned int mult;
> > +   unsigned int val;
> > +   unsigned long rate;
> > +
> > +   val = (clk_readl(zclk->reg) & CPG_FRQCRC_ZFC_MASK)
> > +   >> CPG_FRQCRC_ZFC_SHIFT;
> > +   mult = 32 - val;
> > +
> > +   rate = div_u64((u64)parent_rate * mult + 16, 32);
> > +   /* Round to closest value at 100MHz unit */
> > +   rate = 1*DIV_ROUND_CLOSEST(rate, 1);
> 
> Mike, Stephen: what's your opinion about such rounding tricks?
> 

Is this an actual divider or a pulse skipper?

Peter.