| 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
| ||
|
Message-ID: <33f1ba23ea6698732fb9e869778825ac00a4226e.camel@maquefel.me>
Date: Tue, 20 Jun 2023 15:37:07 +0300
From: Nikita Shubin <nikita.shubin@...uefel.me>
To: andy.shevchenko@...il.com
Cc: Alexander Sverdlin <alexander.sverdlin@...il.com>,
Arnd Bergmann <arnd@...db.de>,
Linus Walleij <linus.walleij@...aro.org>,
Michael Turquette <mturquette@...libre.com>,
Stephen Boyd <sboyd@...nel.org>,
Michael Peters <mpeters@...eddedts.com>,
Kris Bahnsen <kris@...eddedts.com>,
linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org
Subject: Re: [PATCH v1 05/43] clk: ep93xx: add DT support for Cirrus EP93xx
Hello Andy!
Thank you for your review!
On Sat, 2023-06-03 at 21:58 +0300, andy.shevchenko@...il.com wrote:
> Thu, Jun 01, 2023 at 08:33:56AM +0300, Nikita Shubin kirjoitti:
> > This is a rewrite of EP93xx timer driver in
> > arch/arm/mach-ep93xx/clock.c trying to do everything
> > the device tree way:
> >
> > - convert to syscon driver
> > - provide clock acces via of
>
> ...
>
> > +#include <linux/kernel.h>
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/module.h>
> > +#include <linux/string.h>
> > +#include <linux/io.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/clkdev.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/soc/cirrus/ep93xx.h>
>
> Can you keep them sorted?
> Missing bits.h.
>
> + Blank line.
>
> > +#include <dt-bindings/clock/cirrus,ep93xx-clock.h>
> > +
> > +#include <asm/div64.h>
>
> ...
>
> > +static const struct clk_parent_data ep93xx_clk_parents[] = {
> > + EP_PARENT("xtali"),
> > + EP_PARENT("pll1"),
> > + EP_PARENT("pll2")
>
> Keep trailing comma, it might help in case it will be extended.
>
> > +};
>
> ...
>
> > +static unsigned long calc_pll_rate(u64 rate, u32 config_word)
> > +{
> > + int i;
> > +
> > + rate *= ((config_word >> 11) & 0x1f) + 1; /*
> > X1FBD */
> > + rate *= ((config_word >> 5) & 0x3f) + 1; /*
> > X2FBD */
> > + do_div(rate, (config_word & 0x1f) + 1); /*
> > X2IPD */
>
> GENMASK() in all three?
>
> > + for (i = 0; i < ((config_word >> 16) & 3); i++) /*
> > PS */
> > + rate >>= 1;
>
> I'm not sure I understand why loop is needed.
>
> rate >>= 1 << ((config_word >> 16) & GENMASK(1, 0));
>
> ?
>
> > + return rate;
> > +}
>
> ...
>
> > +struct clk_psc {
> > + struct clk_hw hw;
> > + unsigned int reg;
> > + u8 bit_idx;
> > + u32 mask;
> > + u8 shift;
> > + u8 width;
> > + const char *div;
> > + u8 num_div;
> > + spinlock_t *lock;
> > + bool nolock;
>
> Is it important to mix different types like this? pahole can provide
> you a much
> better layout that does not waste a lot of bytes.
>
> > +};
>
> ...
>
> > + return (val & BIT(psc->bit_idx)) ? 1 : 0;
>
> !!(...) also would work, but up to you. Compiler optimizes this
> anyway.
>
> ...
>
> > + unsigned long flags = 0;
>
> Redundant assignment. *spin_lock*() are macros.
> Same for all cases with *spin_lock*().
>
> ...
>
> > +static u8 ep93xx_mux_get_parent(struct clk_hw *hw)
> > +{
> > + struct clk_psc *psc = to_clk_psc(hw);
> > + u32 val;
> > +
> > + ep93xx_regmap_read(psc->reg, &val);
> > + if (!(val & EP93XX_SYSCON_CLKDIV_ESEL))
> > + return 0;
> > +
> > + if (!(val & EP93XX_SYSCON_CLKDIV_PSEL))
> > + return 1;
> > +
> > + return 2;
>
> Wonder if switch-case can make this more explicit...
>
> > +}
>
> ...
>
> > + if (psc->lock)
> > + spin_lock_irqsave(psc->lock, flags);
>
> Does sparse complain on the lock? If so, the function would need a
> special
> annotation.
>
> ...
>
> > + ep93xx_regmap_read(psc->reg, &val);
> > + val &= ~(EP93XX_SYSCON_CLKDIV_ESEL |
> > EP93XX_SYSCON_CLKDIV_PSEL);
> > +
>
> More naturally this blank line looks fter regmap_read.
>
> > + if (index != 0) {
>
> if (index)
>
> also works.
>
> > + val |= EP93XX_SYSCON_CLKDIV_ESEL;
> > + val |= (index - 1) ? EP93XX_SYSCON_CLKDIV_PSEL : 0;
> > + }
>
> ...
>
> > +static unsigned long ep93xx_ddiv_recalc_rate(struct clk_hw *hw,
> > + unsigned long
> > parent_rate)
> > +{
> > + struct clk_psc *psc = to_clk_psc(hw);
> > + unsigned long rate = 0;
>
> Instead you can invert the conditional, see below.
>
> > + u32 val;
> > + int pdiv, div;
> > +
> > + ep93xx_regmap_read(psc->reg, &val);
> > + pdiv = ((val >> EP93XX_SYSCON_CLKDIV_PDIV_SHIFT) & 0x03);
> > + div = val & 0x7f;
>
> GENMASK() in both cases?
Is it a good idea to replace 0x03 with GENMASK(1, 0) ?
>
> > + if (div > 0)
>
> if (div <= 0)
> return 0;
Or even
if (!div)
return 0;
>
> > + rate = DIV_ROUND_CLOSEST(parent_rate * 2, (pdiv +
> > 3) * div);
>
> return DIV_ROUND_CLOSES(...);
>
> > +
> > + return rate;
> > +}
>
> ...
>
> > +static int ep93xx_ddiv_set_rate(struct clk_hw *hw, unsigned long
> > rate,
> > + unsigned long parent_rate)
> > +{
> > + struct clk_psc *psc = to_clk_psc(hw);
> > + int pdiv, div, npdiv, ndiv;
> > + unsigned long actual_rate, mclk_rate, rate_err = -1;
>
> ULONG_MAX instead of -1. -1 on 64-bits is not the same as ULONG_MAX
> (yes, I know that this is not the case here, simply not the best
> constant).
>
> > + int found = 0;
>
> Besides using it as boolean, IIUC it's not needed if you compare
> the rate_err to ULONG_MAX where required.
>
> > + u32 val;
> > +
> > + ep93xx_regmap_read(psc->reg, &val);
> > + mclk_rate = parent_rate * 2;
> > +
> > + for (pdiv = 4; pdiv <= 6; pdiv++) {
> > + div = DIV_ROUND_CLOSEST(mclk_rate, rate * pdiv);
> > + if (div < 1 || div > 127)
> > + continue;
> > +
> > + actual_rate = DIV_ROUND_CLOSEST(mclk_rate, pdiv *
> > div);
>
> > +
>
> Redundant blank line.
>
> > + if (!found || abs(actual_rate - rate) < rate_err) {
> > + npdiv = pdiv - 3;
> > + ndiv = div;
> > + rate_err = abs(actual_rate - rate);
> > + found = 1;
> > + }
> > + }
> > +
> > + if (!found)
> > + return -EINVAL;
>
> > + /* Clear old dividers */
> > + val &= ~0x37f;
>
> GENMASK() ?
>
> > + /* Set the new pdiv and div bits for the new clock rate */
> > + val |= (npdiv << EP93XX_SYSCON_CLKDIV_PDIV_SHIFT) | ndiv;
> > +
> > + ep93xx_syscon_swlocked_write(val, psc->reg);
> > +
> > + return 0;
> > +}
>
> ...
>
> > +{
> > + struct clk_psc *psc = to_clk_psc(hw);
> > + unsigned long best = 0, now;
> > + bool assigned = false;
>
> You see, you are using here the boolean. But think about it, maybe it
> can be
> refactored as well.
>
> > + int i;
> > +
> > + for (i = 0; i < psc->num_div; i++) {
> > + if ((rate * psc->div[i]) == *parent_rate)
> > + return rate;
> > +
> > + now = DIV_ROUND_CLOSEST(*parent_rate, psc->div[i]);
> > +
> > + if (!assigned || is_best(rate, now, best))
> > + best = now;
> > + assigned = true;
> > + }
> > +
> > + return best;
> > +}
>
> ...
>
> > + ep93xx_regmap_read(EP93XX_SYSCON_CLKSET2, &value);
> > + clk_usb_div = (((value >> 28) & 0xf) + 1);
>
> GENMASK() ?
>
> > + hw = clk_hw_register_fixed_factor(NULL, "usb_clk", "pll2",
> > 0, 1, clk_usb_div);
> > + hw = ep93xx_clk_register_gate("ohci-platform",
> > + "usb_clk", 0,
> > + EP93XX_SYSCON_PWRCNT,
> > + EP93XX_SYSCON_PWRCNT_USH_EN,
> > + true);
>
> ...
>
> > + /* pwm clock */
>
> PWM
>
> ...
>
> > + value |= (1 << EP93XX_SYSCON_CLKDIV_PDIV_SHIFT) | 2;
>
> BIT() ?
>
> ...
>
> > + value |= (1 << EP93XX_SYSCON_CLKDIV_PDIV_SHIFT) | 2;
>
> Ditto.
>
> ...
>
> > +static const struct of_device_id ep93xx_clk_dt_ids[] = {
> > + { .compatible = "cirrus,ep9301-clk", },
>
> Inner comma is not needed.
>
> > + { /* sentinel */ }
> > +};
>
> ...
>
> > + ep93xx_clk_data = kzalloc(struct_size(ep93xx_clk_data, hws,
> > + EP93XX_NUM_CLKS),
> > + GFP_KERNEL);
>
> > +
>
> Redundant blank line.
>
> > + if (!ep93xx_clk_data)
> > + return;
>
> ...
>
> > + ret = ep93xx_regmap_read(EP93XX_SYSCON_CHIPID, &value);
> > + if (ret || (value & 0xffff) != EP93XX_SYSCON_CHIPID_ID) {
>
> GENMASK() ?
>
> > + pr_err("failed to read global status register\n");
> > + return;
> > + }
>
> ...
>
> > + /* Initialize the pll1 derived clocks */
> > + clk_f_div = fclk_divisors[(value >> 25) & 0x7];
> > + clk_h_div = hclk_divisors[(value >> 20) & 0x7];
> > + clk_p_div = pclk_divisors[(value >> 18) & 0x3];
>
> Ditto.
>
>
> ...
>
> > +
>
> Unneded blank line.
>
> > +CLK_OF_DECLARE_DRIVER(ep93xx, "cirrus,ep9301-clk",
> > ep93xx_clock_init);
>
Powered by blists - more mailing lists