lkml.org 
[lkml]   [2023]   [Jun]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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


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