Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1735330 > unrolled thread

Re: [PATCH 1/7] PM / OPP: Add platform specific set_clk function

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/7] PM / OPP: Add platform specific set_clk function Viresh Kumar <viresh.kumar@linaro.org> - 2017-09-20 01:00 +0200
    Re: [PATCH 1/7] PM / OPP: Add platform specific set_clk function Dong Aisheng <dongas86@gmail.com> - 2017-09-20 09:10 +0200
      Re: [PATCH 1/7] PM / OPP: Add platform specific set_clk function Viresh Kumar <viresh.kumar@linaro.org> - 2017-09-20 22:40 +0200
        RE: [PATCH 1/7] PM / OPP: Add platform specific set_clk function "A.s. Dong" <aisheng.dong@nxp.com> - 2017-09-21 04:20 +0200

#1735330 — Re: [PATCH 1/7] PM / OPP: Add platform specific set_clk function

On 24-08-17, 00:10, Dong Aisheng wrote:
> This is useful to support platforms which only the clk setting is
> different from the generic OPP set rate but others like voltage
> setting are still the same.
> 
> Users can use this function to register a custom OPP set clk helper
> working in place of the default simple clk setting in the generic
> dev_pm_opp_set_rate(). Then user can still use dev_pm_opp_set_rate()
> with .set_clk() to save a lot duplicated work.

I am not inclined to add this support really. What prevents you to
register a clock for the device (which is CPU in your case) and the
generic clk_set_rate() will eventually call into the platform specific
routine. That's what everyone else is doing.

-- 
viresh

[toc] | [next] | [standalone]

#1735571

Hi Viresh,

On Tue, Sep 19, 2017 at 03:58:40PM -0700, Viresh Kumar wrote:
> On 24-08-17, 00:10, Dong Aisheng wrote:
> > This is useful to support platforms which only the clk setting is
> > different from the generic OPP set rate but others like voltage
> > setting are still the same.
> > 
> > Users can use this function to register a custom OPP set clk helper
> > working in place of the default simple clk setting in the generic
> > dev_pm_opp_set_rate(). Then user can still use dev_pm_opp_set_rate()
> > with .set_clk() to save a lot duplicated work.
> 
> I am not inclined to add this support really. What prevents you to
> register a clock for the device (which is CPU in your case) and the
> generic clk_set_rate() will eventually call into the platform specific
> routine. That's what everyone else is doing.
> 

I've been thinking of that before.
Actually IMX already does some similar thing for MX5 (no for MX6).
See: clk_cpu_set_rate() in drivers/clk/imx/clk-cpu.c.

After some diggings, it seems MX7ULP is a bit more complicated than before
mainly due to two reasons:
1) It requires to switch to different CPU mode accordingly when setting
clocks rate. That means we need handle this in clock driver as well
which looks not quite suitable although we could do if really want.

2) It uses different clocks for different CPU mode (RUN 416M or
HSRUN 528M), and those clocks have some dependency.
e.g. when setting HSRUN clock, we need change RUN clock parent to make sure
the SPLL_PFD is got disabled before changing rate, as both CPU mode using
the same parent SPLL_PFD clock. Doing this in clock driver also make things
a bit more complicated.

The whole follow would be something like below:
static int imx7ulp_set_clk(struct device *dev, struct clk *clk,
                           unsigned long old_freq, unsigned long new_freq)
{
        u32 val;

        /*
         * Before changing the ARM core PLL, change the ARM clock soure
         * to FIRC first.
         */
        if (new_freq >= HSRUN_FREQ) {
                clk_set_parent(clks[RUN_SCS_SEL].clk, clks[FIRC].clk);

                /* switch to HSRUN mode */
                val = readl_relaxed(smc_base + SMC_PMCTRL);
                val |= (0x3 << 8);
                writel_relaxed(val, smc_base + SMC_PMCTRL);

                /* change the clock rate in HSRUN */
                clk_set_rate(clks[SPLL_PFD0].clk, new_freq);
                clk_set_parent(clks[HSRUN_SCS_SEL].clk, clks[SPLL_SEL].clk);
        } else {
                /* change the HSRUN clock to firc */
                clk_set_parent(clks[HSRUN_SCS_SEL].clk, clks[FIRC].clk);

                /* switch to RUN mode */
                val = readl_relaxed(smc_base + SMC_PMCTRL);
                val &= ~(0x3 << 8);
                writel_relaxed(val, smc_base + SMC_PMCTRL);

                clk_set_rate(clks[SPLL_PFD0].clk, new_freq);
                clk_set_parent(clks[RUN_SCS_SEL].clk, clks[SPLL_SEL].clk);
        }

        return 0;
}

That's why i thought if we can make OPP core provide a way to handle such
complicated things in platform specific cpufreq driver.

How would you suggest for this issue?

Regards
Dong Aisheng

> -- 
> viresh

[toc] | [prev] | [next] | [standalone]

#1736091

On 20-09-17, 15:03, Dong Aisheng wrote:
> I've been thinking of that before.
> Actually IMX already does some similar thing for MX5 (no for MX6).
> See: clk_cpu_set_rate() in drivers/clk/imx/clk-cpu.c.
> 
> After some diggings, it seems MX7ULP is a bit more complicated than before
> mainly due to two reasons:
> 1) It requires to switch to different CPU mode accordingly when setting
> clocks rate. That means we need handle this in clock driver as well
> which looks not quite suitable although we could do if really want.
> 
> 2) It uses different clocks for different CPU mode (RUN 416M or
> HSRUN 528M), and those clocks have some dependency.
> e.g. when setting HSRUN clock, we need change RUN clock parent to make sure
> the SPLL_PFD is got disabled before changing rate, as both CPU mode using
> the same parent SPLL_PFD clock. Doing this in clock driver also make things
> a bit more complicated.
> 
> The whole follow would be something like below:
> static int imx7ulp_set_clk(struct device *dev, struct clk *clk,
>                            unsigned long old_freq, unsigned long new_freq)
> {
>         u32 val;
> 
>         /*
>          * Before changing the ARM core PLL, change the ARM clock soure
>          * to FIRC first.
>          */
>         if (new_freq >= HSRUN_FREQ) {
>                 clk_set_parent(clks[RUN_SCS_SEL].clk, clks[FIRC].clk);
> 
>                 /* switch to HSRUN mode */
>                 val = readl_relaxed(smc_base + SMC_PMCTRL);
>                 val |= (0x3 << 8);
>                 writel_relaxed(val, smc_base + SMC_PMCTRL);
> 
>                 /* change the clock rate in HSRUN */
>                 clk_set_rate(clks[SPLL_PFD0].clk, new_freq);
>                 clk_set_parent(clks[HSRUN_SCS_SEL].clk, clks[SPLL_SEL].clk);
>         } else {
>                 /* change the HSRUN clock to firc */
>                 clk_set_parent(clks[HSRUN_SCS_SEL].clk, clks[FIRC].clk);
> 
>                 /* switch to RUN mode */
>                 val = readl_relaxed(smc_base + SMC_PMCTRL);
>                 val &= ~(0x3 << 8);
>                 writel_relaxed(val, smc_base + SMC_PMCTRL);
> 
>                 clk_set_rate(clks[SPLL_PFD0].clk, new_freq);
>                 clk_set_parent(clks[RUN_SCS_SEL].clk, clks[SPLL_SEL].clk);
>         }
> 
>         return 0;
> }

Right and we have the same thing in the cpufreq driver now. It will
stay at some place and we need to find the best one, keeping in mind
that we may or may not want to solve this problem in a generic way.

> That's why i thought if we can make OPP core provide a way to handle such
> complicated things in platform specific cpufreq driver.
> 
> How would you suggest for this issue?

I wouldn't add an API into the OPP framework if I were you. There is
just too much code to add to the core to handle such platform specific
stuff, which you are anyway going to keep somewhere as it is. IMHO,
keeping that in the clock driver is a better thing to do than this.

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1736331

> -----Original Message-----
> From: Viresh Kumar [mailto:viresh.kumar@linaro.org]
> Sent: Thursday, September 21, 2017 4:31 AM
> To: Dong Aisheng
> Cc: A.s. Dong; linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; sboyd@codeaurora.org;
> vireshk@kernel.org; nm@ti.com; rjw@rjwysocki.net; shawnguo@kernel.org;
> Anson Huang; Jacky Bai
> Subject: Re: [PATCH 1/7] PM / OPP: Add platform specific set_clk function
> 
> On 20-09-17, 15:03, Dong Aisheng wrote:
> > I've been thinking of that before.
> > Actually IMX already does some similar thing for MX5 (no for MX6).
> > See: clk_cpu_set_rate() in drivers/clk/imx/clk-cpu.c.
> >
> > After some diggings, it seems MX7ULP is a bit more complicated than
> > before mainly due to two reasons:
> > 1) It requires to switch to different CPU mode accordingly when
> > setting clocks rate. That means we need handle this in clock driver as
> > well which looks not quite suitable although we could do if really want.
> >
> > 2) It uses different clocks for different CPU mode (RUN 416M or HSRUN
> > 528M), and those clocks have some dependency.
> > e.g. when setting HSRUN clock, we need change RUN clock parent to make
> > sure the SPLL_PFD is got disabled before changing rate, as both CPU
> > mode using the same parent SPLL_PFD clock. Doing this in clock driver
> > also make things a bit more complicated.
> >
> > The whole follow would be something like below:
> > static int imx7ulp_set_clk(struct device *dev, struct clk *clk,
> >                            unsigned long old_freq, unsigned long
> > new_freq) {
> >         u32 val;
> >
> >         /*
> >          * Before changing the ARM core PLL, change the ARM clock soure
> >          * to FIRC first.
> >          */
> >         if (new_freq >= HSRUN_FREQ) {
> >                 clk_set_parent(clks[RUN_SCS_SEL].clk, clks[FIRC].clk);
> >
> >                 /* switch to HSRUN mode */
> >                 val = readl_relaxed(smc_base + SMC_PMCTRL);
> >                 val |= (0x3 << 8);
> >                 writel_relaxed(val, smc_base + SMC_PMCTRL);
> >
> >                 /* change the clock rate in HSRUN */
> >                 clk_set_rate(clks[SPLL_PFD0].clk, new_freq);
> >                 clk_set_parent(clks[HSRUN_SCS_SEL].clk,
> clks[SPLL_SEL].clk);
> >         } else {
> >                 /* change the HSRUN clock to firc */
> >                 clk_set_parent(clks[HSRUN_SCS_SEL].clk,
> > clks[FIRC].clk);
> >
> >                 /* switch to RUN mode */
> >                 val = readl_relaxed(smc_base + SMC_PMCTRL);
> >                 val &= ~(0x3 << 8);
> >                 writel_relaxed(val, smc_base + SMC_PMCTRL);
> >
> >                 clk_set_rate(clks[SPLL_PFD0].clk, new_freq);
> >                 clk_set_parent(clks[RUN_SCS_SEL].clk,
> clks[SPLL_SEL].clk);
> >         }
> >
> >         return 0;
> > }
> 
> Right and we have the same thing in the cpufreq driver now. It will stay
> at some place and we need to find the best one, keeping in mind that we
> may or may not want to solve this problem in a generic way.
> 
> > That's why i thought if we can make OPP core provide a way to handle
> > such complicated things in platform specific cpufreq driver.
> >
> > How would you suggest for this issue?
> 
> I wouldn't add an API into the OPP framework if I were you. There is just
> too much code to add to the core to handle such platform specific stuff,
> which you are anyway going to keep somewhere as it is. IMHO, keeping that
> in the clock driver is a better thing to do than this.
> 

Okay, I will give a try in CLK driver.
Thanks for the suggestion.

Regards
Dong Aisheng

> --
> viresh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web