Messages in this thread | | | Subject | Re: [PATCH v1 05/43] clk: ep93xx: add DT support for Cirrus EP93xx | From | Nikita Shubin <> | Date | Tue, 20 Jun 2023 15:37:07 +0300 |
| |
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); >
| |