lkml.org 
[lkml]   [2023]   [Jun]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v1 05/43] clk: ep93xx: add DT support for Cirrus EP93xx
From
Date
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);
>

\
 
 \ /
  Last update: 2023-06-20 14:38    [W:0.152 / U:1.972 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site