| 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: <ZHuNXDjPfOvyTNtp@surfacebook>
Date: Sat, 3 Jun 2023 21:58:36 +0300
From: andy.shevchenko@...il.com
To: Nikita Shubin <nikita.shubin@...uefel.me>
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
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?
> + if (div > 0)
if (div <= 0)
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);
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists