linux-kernel - Re: [PATCH v4 3/7] clk: kona: don't init clocks at startup time

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140530233702.10062.60452@quantum>
Date:	Fri, 30 May 2014 16:37:02 -0700
From:	Mike Turquette <mturquette@...aro.org>
To:	Alex Elder <elder@...aro.org>, mporter@...aro.org,
	bcm@...thebug.org
Cc:	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 3/7] clk: kona: don't init clocks at startup time

Quoting Alex Elder (2014-05-30 13:53:04)
> +static int kona_clk_prepare(struct clk_hw *hw)
>  {
> +       struct kona_clk *bcm_clk = to_kona_clk(hw);
> +       struct ccu_data *ccu = bcm_clk->ccu;
> +       unsigned long flags;
> +       int ret = 0;
> +
> +       flags = ccu_lock(ccu);
> +       __ccu_write_enable(ccu);
> +
>         switch (bcm_clk->type) {
>         case bcm_clk_peri:
> -               return __peri_clk_init(bcm_clk);
> +               if (!__peri_clk_init(bcm_clk))
> +                       ret = -EINVAL;
> +               break;
>         default:
>                 BUG();
>         }

The switch-case only has one match, plus a default. Will there be others
in the future?  Otherwise it can be replaced with an if-statement.

> -       return -EINVAL;
> -}
> -
> -/* Set a CCU and all its clocks into their desired initial state */
> -bool __init kona_ccu_init(struct ccu_data *ccu)
> -{
> -       unsigned long flags;
> -       unsigned int which;
> -       struct clk **clks = ccu->clk_data.clks;
> -       bool success = true;
> -
> -       flags = ccu_lock(ccu);
> -       __ccu_write_enable(ccu);
> -
> -       for (which = 0; which < ccu->clk_data.clk_num; which++) {
> -               struct kona_clk *bcm_clk;
> -
> -               if (!clks[which])
> -                       continue;
> -               bcm_clk = to_kona_clk(__clk_get_hw(clks[which]));
> -               success &= __kona_clk_init(bcm_clk);
> -       }
>  
>         __ccu_write_disable(ccu);
>         ccu_unlock(ccu, flags);
> -       return success;
> +
> +       return ret;
>  }

Does this prepare callback "enable" a clock? E.g does a line NOT toggle
at a rate prior to this call, and then after this call completes that
same line is now toggling at a rate?

>  
> -/* Clock operations */
> +static void kona_clk_unprepare(struct clk_hw *hw)
> +{
> +       /* Nothing to do. */
> +}

Is doing nothing the right thing to do? Could power be saved somehow if
the .unprepare callback really gets called? Remember that if .unprepare
actually runs (because struct clk->prepare_count == 0) then the next
call to clk_prepare will actually call your .prepare callback and set up
the prereq clocks again. So the prereq clock initialization is no longer
a one-time thing, which might afford you some optimizations.

Regards,
Mike

>  
>  static int kona_peri_clk_enable(struct clk_hw *hw)
>  {
> @@ -1264,6 +1258,8 @@ static int kona_peri_clk_set_rate(struct clk_hw *hw, unsigned long rate,
>  }
>  
>  struct clk_ops kona_peri_clk_ops = {
> +       .prepare = kona_clk_prepare,
> +       .unprepare = kona_clk_unprepare,
>         .enable = kona_peri_clk_enable,
>         .disable = kona_peri_clk_disable,
>         .is_enabled = kona_peri_clk_is_enabled,
> diff --git a/drivers/clk/bcm/clk-kona.h b/drivers/clk/bcm/clk-kona.h
> index e9a8466..3409111 100644
> --- a/drivers/clk/bcm/clk-kona.h
> +++ b/drivers/clk/bcm/clk-kona.h
> @@ -511,6 +511,5 @@ extern u64 scaled_div_build(struct bcm_clk_div *div, u32 div_value,
>  extern struct clk *kona_clk_setup(struct kona_clk *bcm_clk);
>  extern void __init kona_dt_ccu_setup(struct ccu_data *ccu,
>                                 struct device_node *node);
> -extern bool __init kona_ccu_init(struct ccu_data *ccu);
>  
>  #endif /* _CLK_KONA_H */
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/