| From | andy.shevchenko@gmail ... | Date | Sat, 3 Jun 2023 21:58:36 +0300 | Subject | Re: [PATCH v1 05/43] clk: ep93xx: add DT support for Cirrus EP93xx |
| |
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?
> + if (div > 0)
if (div <= 0) 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);
-- With Best Regards, Andy Shevchenko
|