| 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: <466da2894d7d5f2a23047c812dd34b52034402f8.camel@maquefel.me>
Date: Sat, 23 Dec 2023 11:35:07 +0300
From: Nikita Shubin <nikita.shubin@...uefel.me>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
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
Hello Andy!
On Wed, 2023-12-13 at 19:42 +0200, Andy Shevchenko wrote:
> 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?
It was my thought on first iterations of clk, unfortunately i don't see
any good way to combine them:
- mux_determine_rate is searching the best parent that meet the
criteria
- set_rate is searching the actual dividers to write, so difference is
minimum
Combining them into one doesn't make it more understandable.
>
> ...
>
> > + /*
> > + * 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
> > + */
>
Powered by blists - more mailing lists