Re: [PATCH v1 05/43] clk: ep93xx: add DT support for Cirrus EP93xx

[Nikita Shubin <nikita.shubin@maquefel.me>]

Hello Andy!

Thank you for your review!

On Sat, 2023-06-03 at 21:58 +0300, andy.shevchenko@gmail.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);
>