| 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
| ||
|
Date: Wed, 13 Dec 2023 19:42:14 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Nikita Shubin <nikita.shubin@...uefel.me>
Cc: Michael Turquette <mturquette@...libre.com>,
Stephen Boyd <sboyd@...nel.org>, linux-kernel@...r.kernel.org,
linux-clk@...r.kernel.org, Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH v6 04/40] clk: ep93xx: add DT support for Cirrus EP93xx
On Tue, Dec 12, 2023 at 11:20:21AM +0300, Nikita Shubin wrote:
> Rewrite EP93xx clock driver located in arch/arm/mach-ep93xx/clock.c
> trying to do everything the device tree way:
>
> - provide clock acces via of
> - drop clk_hw_register_clkdev
> - drop init code and use module_auxiliary_driver
...
> +#define EP93XX_I2SCLKDIV_SDIV (1 << 16)
BIT() ?
...
> +static u8 ep93xx_mux_get_parent(struct clk_hw *hw)
> +{
> + struct ep93xx_clk *clk = ep93xx_clk_from(hw);
> + struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk);
> + u32 val;
> +
> + regmap_read(priv->map, clk->reg, &val);
> +
> + val &= EP93XX_SYSCON_CLKDIV_MASK;
> +
> + switch (val) {
> + case EP93XX_SYSCON_CLKDIV_ESEL:
> + return 1; /* PLL1 */
> + case EP93XX_SYSCON_CLKDIV_MASK:
> + return 2; /* PLL2 */
> + default:
> + break;
> + };
> +
> + return 0; /* XTALI */
You may return directly from default.
> +}
...
> +static int ep93xx_mux_set_parent_lock(struct clk_hw *hw, u8 index)
> +{
> + struct ep93xx_clk *clk = ep93xx_clk_from(hw);
> + struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk);
> + unsigned long flags;
> + u32 val;
> +
> + if (index >= 3)
> + return -EINVAL;
> + spin_lock_irqsave(&priv->lock, flags);
Why not guard() ?
> + regmap_read(priv->map, clk->reg, &val);
> + val &= ~(EP93XX_SYSCON_CLKDIV_MASK);
> + val |= index > 0 ? EP93XX_SYSCON_CLKDIV_ESEL : 0;
> + val |= index > 1 ? EP93XX_SYSCON_CLKDIV_PSEL : 0;
> +
> + ep93xx_clk_write(priv, clk->reg, val);
> +
> + spin_unlock_irqrestore(&priv->lock, flags);
> +
> + return 0;
> +}
...
> +static bool is_best(unsigned long rate, unsigned long now,
> + unsigned long best)
> +{
> + return abs_diff(rate, now) < abs_diff(rate, best);
Have you included necessary header for this?
> +}
...
> +static int ep93xx_mux_determine_rate(struct clk_hw *hw,
> + struct clk_rate_request *req)
> +{
> + unsigned long best_rate = 0, actual_rate, mclk_rate;
> + unsigned long rate = req->rate;
> + struct clk_hw *parent_best = NULL;
Strictly speaking you don't need an assignment here as you can compare the loop
variable value against the maximum. But I don't know how heave the respective
CLk call is and if it has no side-effects due to operations inside the loop body.
> + unsigned long parent_rate_best;
> + unsigned long parent_rate;
> + int div, pdiv;
> + unsigned int i;
> +
> + /*
> + * Try the two pll's and the external clock
Either comma + 'b' or missing period.
> + * Because the valid predividers are 2, 2.5 and 3, we multiply
> + * all the clocks by 2 to avoid floating point math.
> + *
> + * This is based on the algorithm in the ep93xx raster guide:
> + * http://be-a-maverick.com/en/pubs/appNote/AN269REV1.pdf
> + *
> + */
> + for (i = 0; i < clk_hw_get_num_parents(hw); i++) {
> + struct clk_hw *parent = clk_hw_get_parent_by_index(hw, i);
> +
> + parent_rate = clk_hw_get_rate(parent);
> + mclk_rate = parent_rate * 2;
> +
> + /* Try each predivider value */
> + for (pdiv = 4; pdiv <= 6; pdiv++) {
> + div = DIV_ROUND_CLOSEST(mclk_rate, rate * pdiv);
> + if (!in_range(div, 1, 127))
Same header as for abs_diff()?
> + continue;
> +
> + actual_rate = DIV_ROUND_CLOSEST(mclk_rate, pdiv * div);
> + if (is_best(rate, actual_rate, best_rate)) {
> + best_rate = actual_rate;
> + parent_rate_best = parent_rate;
> + parent_best = parent;
> + }
> + }
(1)
> + }
> +
> + if (!parent_best)
> + return -EINVAL;
> +
> + req->best_parent_rate = parent_rate_best;
> + req->best_parent_hw = parent_best;
> + req->rate = best_rate;
> +
> + return 0;
> +}
...
> + mclk_rate = parent_rate * 2;
> +
> + for (pdiv = 4; pdiv <= 6; pdiv++) {
> + div = DIV_ROUND_CLOSEST(mclk_rate, rate * pdiv);
> + if (!in_range(div, 1, 127))
> + continue;
> +
> + actual_rate = DIV_ROUND_CLOSEST(mclk_rate, pdiv * div);
> + if (abs(actual_rate - rate) < rate_err) {
> + npdiv = pdiv - 3;
> + ndiv = div;
> + rate_err = abs(actual_rate - rate);
> + }
> + }
Looks very similar to (1). Can be deduplicated?
...
> + /*
> + * Clear old dividers
> + * Bit 7 is reserved bit in all ClkDiv registers
Missing periods.
> + */
...
> +static unsigned long calc_pll_rate(u64 rate, u32 config_word)
> +{
> + rate *= ((config_word >> 11) & GENMASK(4, 0)) + 1; /* X1FBD */
> + rate *= ((config_word >> 5) & GENMASK(5, 0)) + 1; /* X2FBD */
> + do_div(rate, (config_word & GENMASK(4, 0)) + 1); /* X2IPD */
> + rate >>= ((config_word >> 16) & GENMASK(1, 0)); /* PS */
Outer parentheses are redundant.
> + return rate;
> +}
...
> + /*
> + * EP93xx SSP clock rate was doubled in version E2. For more information
> + * see:
> + * http://www.cirrus.com/en/pubs/appNote/AN273REV4.pdf
Can you point to the specific section? Like
* see section 1.2.3 "Foo bar":
> + */
...
> + /* touchscreen/adc clock */
ADC
...
> + /*
> + * On reset PDIV and VDIV is set to zero, while PDIV zero
> + * means clock disable, VDIV shouldn't be zero.
> + * So we set both video and is2 dividers to minimum.
i2s?
> + * ENA - Enable CLK divider.
> + * PDIV - 00 - Disable clock
> + * VDIV - at least 2
> + */
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists