Re: [PATCH v6 04/40] clk: ep93xx: add DT support for Cirrus EP93xx

From: Nikita Shubin <nikita.shubin@maquefel.me>
To: Andy Shevchenko <andriy.shevchenko@intel.com>
Cc: Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	 linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org,
	Arnd Bergmann <arnd@arndb.de>
Subject: Re: [PATCH v6 04/40] clk: ep93xx: add DT support for Cirrus EP93xx
Date: Sat, 23 Dec 2023 11:35:07 +0300	[thread overview]
Message-ID: <466da2894d7d5f2a23047c812dd34b52034402f8.camel@maquefel.me> (raw)
In-Reply-To: <ZXns9klLAhuK-Alz@smile.fi.intel.com>

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
> > +        */
> 