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] [day] [month] [year] [list]
Date:	Fri, 07 Sep 2012 18:22:58 -0700
From:	Mike Turquette <mturquette@...aro.org>
To:	Thierry Reding <thierry.reding@...onic-design.de>,
	Guan Xuetao <gxt@...c.pku.edu.cn>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/6] unicore32: Add common clock support

Quoting Thierry Reding (2012-09-02 03:21:11)
> diff --git a/arch/unicore32/kernel/clock.c b/arch/unicore32/kernel/clock.c
<snip>
> +struct clk_uc {
> +       struct clk_hw hw;
>  };

This looks ugly.  Normally register addresses, masks and the like would
go here.  Instead you duplicate some functions below (which should be
common) and hard-code the addresses in those functions.  Below I'll use
your recalc_rate as an example...

<snip>
> +static inline struct clk_uc *to_clk_uc(struct clk_hw *hw)
>  {
> +       return container_of(hw, struct clk_uc, hw);
>  }

This is never used.  It should be, but it is not.  I'll address that
more below.

<snip>
> +static struct clk *clk_register_uc(const char *name, const char *parent,
> +                                  const struct clk_ops *ops)
>  {
> -       if (clk == &clk_vga_clk) {
> -               unsigned long pll_vgacfg, pll_vgadiv;
> -               int ret, i;
> -
> -               /* lookup vga_clk_table */
> -               ret = -EINVAL;
> -               for (i = 0; i < ARRAY_SIZE(vga_clk_table); i++) {
> -                       if (rate == vga_clk_table[i].rate) {
> -                               pll_vgacfg = vga_clk_table[i].cfg;
> -                               pll_vgadiv = vga_clk_table[i].div;
> -                               ret = 0;
> -                               break;
> -                       }
> -               }
> -
> -               if (ret)
> -                       return ret;
> -
> -               if (readl(PM_PLLVGACFG) == pll_vgacfg)
> -                       return 0;
> +       struct clk_init_data init;
> +       struct clk_uc *uc;
> +       struct clk *clk;
>  
> -               /* set pll vga cfg reg. */
> -               writel(pll_vgacfg, PM_PLLVGACFG);
> +       uc = kzalloc(sizeof(*uc), GFP_KERNEL);
> +       if (!uc)
> +               return NULL;

-ENOMEM?

<snip>
> +static unsigned long clk_vga_recalc_rate(struct clk_hw *hw,
> +                                        unsigned long parent_rate)
> +{
> +       unsigned long pllrate = readl(PM_PLLVGASTATUS);
> +       unsigned long divstatus = readl(PM_DIVSTATUS);
> +       unsigned long rate = 0;
> +       unsigned int i;
> +
> +       /* lookup pvga_table */
> +       for (i = 0; i < ARRAY_SIZE(pllrate_table); i++) {
> +               if (pllrate == pllrate_table[i].prate) {
> +                       rate = pllrate_table[i].rate;
> +                       break;
> +               }
> +       }
> +
> +       if (rate)
> +               rate = rate / (((divstatus & 0x00f00000) >> 20) + 1);
> +
> +       return rate;
> +}

This vga recalc_rate code seems very similar to the mclk and ddr
recalc_rate functions (found further down below).  It would be better to
do something like this:

		struct clk_uc *uc = to_clk_uc(hw);
		unsigned long pllrate = readl(uc->pll_status);
		...

This means that you can reuse the same function for vga, mclk and ddr
clocks.  The same logic can be extrapolated to apply to some other bits
of code in here as well, I think.

<snip>
> +static unsigned long clk_mclk_recalc_rate(struct clk_hw *hw,
> +                                         unsigned long parent_rate)
> +{
> +#ifndef CONFIG_ARCH_FPGA
> +       unsigned long pllrate = readl(PM_PLLSYSSTATUS);
> +       unsigned long rate = 0;
> +       unsigned int i;
> +
> +       /* lookup pllrate_table */
> +       for (i = 0; i < ARRAY_SIZE(pllrate_table); i++) {
> +               if (pllrate == pllrate_table[i].prate) {
> +                       rate = pllrate_table[i].rate;
> +                       break;
> +               }
> +       }
> +
> +       return rate;
> +#else
> +       return 33000000;
> +#endif
> +}

If ARCH_FPGA is defined then register a fixed-rate clock (see
drivers/clk/clk-fixed-rate.c).  Then you won't need ifdefs in these
functions and it will help you consolidate (as mentioned above).  This
same pattern repeats a few times throughout the code.

Regards,
Mike
--
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/

Powered by blists - more mailing lists