Re: [PATCH v3 04/10] clk: clock-wizard: Add support for dynamic reconfiguration

Quoting shubhrajyoti.datta@gmail.com (2019-11-27 22:36:11)

From: Shubhrajyoti Datta <redacted>

The patch adds support for dynamic reconfiguration of clock output rate.
Output clocks are registered as dividers and set rate callback function
is used for dynamic reconfiguration.

Based on the initial work from Chirag.

Signed-off-by: Chirag Parekh <redacted>
Signed-off-by: Shubhrajyoti Datta <redacted>
Signed-off-by: Michal Simek <redacted>

Are these more like Co-developed-by: tags?

Is this register lock used by anything? I'm mostly wondering if dividers
are in a shared register with some other clk so we need to have this
lock. Otherwise it adds more conditional locking code for no gain and
should be removed to simplify.

Is this casting necessary?

+       unsigned int val;
+
+       val = readl(div_addr) >> divider->shift;
+       val &= div_mask(divider->width);
+
+       return divider_recalc_rate(hw, parent_rate, val, divider->table,
+                       divider->flags, divider->width);
+}
+
+static int clk_wzrd_dynamic_reconfig(struct clk_hw *hw, unsigned long rate,
+                                    unsigned long parent_rate)
+{
+       int err = 0;
+       u16 retries;
+       u32 value;
+       unsigned long flags = 0;
+       struct clk_wzrd_divider *divider = to_clk_wzrd_divider(hw);
+       void __iomem *div_addr =
+                       (void __iomem *)((u64)divider->base + divider->offset);
+
+       if (divider->lock)
+               spin_lock_irqsave(divider->lock, flags);
+       else
+               __acquire(divider->lock);
+
+       value = DIV_ROUND_CLOSEST(parent_rate, rate);
+
+       /* Cap the value to max */
+       if (value > WZRD_DR_MAX_INT_DIV_VALUE)
+               value = WZRD_DR_MAX_INT_DIV_VALUE;

Please use min().

+
+       /* Set divisor and clear phase offset */
+       writel(value, div_addr);
+       writel(0x00, div_addr + WZRD_DR_DIV_TO_PHASE_OFFSET);
+
+       /* Check status register */
+       retries = WZRD_DR_NUM_RETRIES;
+       while (retries--) {
+               if (readl(divider->base + WZRD_DR_STATUS_REG_OFFSET) &
+                                                       WZRD_DR_LOCK_BIT_MASK)
+                       break;
+       }

Is this readl_poll_timeout()?

+
+       if (retries == 0) {
+               err = -ETIMEDOUT;
+               goto err_reconfig;
+       }
+
+       /* Initiate reconfiguration */
+       writel(WZRD_DR_BEGIN_DYNA_RECONF,
+              divider->base + WZRD_DR_INIT_REG_OFFSET);
+
+       /* Check status register */
+       retries = WZRD_DR_NUM_RETRIES;
+       while (retries--) {
+               if (readl(divider->base + WZRD_DR_STATUS_REG_OFFSET) &
+                                                       WZRD_DR_LOCK_BIT_MASK)
+                       break;
+       }
+
+       if (retries == 0)
+               err = -ETIMEDOUT;

readl_poll_timeout()?

+
+err_reconfig:
+       if (divider->lock)
+               spin_unlock_irqrestore(divider->lock, flags);
+       else
+               __release(divider->lock);
+
+       return err;
+}
+
+static long clk_wzrd_round_rate(struct clk_hw *hw, unsigned long rate,
+                               unsigned long *prate)
+{
+       u8 div;
+
+       /*
+        * since we donot change parent rate we just round rate to closest

s/donot/don't/

+        * achievable
+        */
+       div = DIV_ROUND_CLOSEST(*prate, rate);
+
+       return (*prate / div);
+}
+
+static const struct clk_ops clk_wzrd_clk_divider_ops = {
+       .round_rate = clk_wzrd_round_rate,
+       .set_rate = clk_wzrd_dynamic_reconfig,
+       .recalc_rate = clk_wzrd_recalc_rate,
+};
+
+static struct clk *clk_wzrd_register_divider(struct device *dev,
+                                            const char *name,
+                                            const char *parent_name,
+                                            unsigned long flags,
+                                            void __iomem *base, u16 offset,
+                                            u8 shift, u8 width,
+                                            u8 clk_divider_flags,
+                                            const struct clk_div_table *table,
+                                            spinlock_t *lock)
+{
+       struct clk_wzrd_divider *div;
+       struct clk_hw *hw;
+       struct clk_init_data init;
+       int ret;
+
+       if (clk_divider_flags & CLK_DIVIDER_HIWORD_MASK) {

Do you use HIWORD mask things? I thought that was mostly rockchip
specific quirk. If not, please remove this code.

+               if (width + shift > 16) {
+                       pr_warn("divider value exceeds LOWORD field\n");
+                       return ERR_PTR(-EINVAL);
+               }
+       }
+
+       /* allocate the divider */
+       div = kzalloc(sizeof(*div), GFP_KERNEL);
+       if (!div)
+               return ERR_PTR(-ENOMEM);
+
+       init.name = name;
+       if (clk_divider_flags & CLK_DIVIDER_READ_ONLY)
+               init.ops = &clk_divider_ro_ops;
+       else
+               init.ops = &clk_wzrd_clk_divider_ops;
+       init.flags = flags | CLK_IS_BASIC;

Oh yeah, don't add this flag. This code won't compile so please compile
test each patch in a series to make sure we don't have compilation
bisection holes.

+       init.parent_names = (parent_name ? &parent_name : NULL);
+       init.num_parents = (parent_name ? 1 : 0);

Is it sometimes the root of the tree? Seems unlikely so probably just
assume there is a parent all the time.

+
+       /* struct clk_divider assignments */

Drop useless comment please.

+       div->base = base;
+       div->offset = offset;
+       div->shift = shift;
+       div->width = width;
+       div->flags = clk_divider_flags;
+       div->lock = lock;
+       div->hw.init = &init;
+       div->table = table;
+
+       /* register the clock */

Drop useless comment please.

Please remove useless parenthesis.

+                               WZRD_CLKOUT_DIVIDE_SHIFT,
+                               WZRD_CLKOUT_DIVIDE_WIDTH,
+                               CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ALLOW_ZERO,
+                               NULL, &clkwzrd_lock);
                if (IS_ERR(clk_wzrd->clkout[i])) {
                        int j;
 
-- 
2.1.1