Re: [PATCH v6 09/12] clk: nuvoton: Add clock driver for ma35d1 clock controller

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Dear Stephen,


Thanks for your advice.

On 2023/3/29 上午 02:18, Stephen Boyd wrote:
Quoting Jacky Huang (2023-03-27 19:19:09)
diff --git a/drivers/clk/nuvoton/Kconfig b/drivers/clk/nuvoton/Kconfig
new file mode 100644
index 000000000000..c1324efedcb9
--- /dev/null
+++ b/drivers/clk/nuvoton/Kconfig
@@ -0,0 +1,19 @@
+# SPDX-License-Identifier: GPL-2.0
+# common clock support for Nuvoton SoC family.
+
+config COMMON_CLK_NUVOTON
+       bool "Nuvoton clock controller common support"
+       depends on ARCH_NUVOTON
Add || COMPILE_TEST

+       default ARCH_NUVOTON
+       help
+         Say y here to enable common clock controller for Nuvoton platforms.
+
+if COMMON_CLK_NUVOTON
Add a newline here.

+config CLK_MA35D1
+       bool "Nuvoton MA35D1 clock controller support"
+       depends on ARM64 || COMPILE_TEST
Drop this

+       default y
And this.

+       help
+         Build the driver for MA35D1 Clock Driver.
This says driver twice. "Build the clk driver for MA35D1 SoCs"?

I will fix all above of Kconfig file. Thank you.

+
+endif
diff --git a/drivers/clk/nuvoton/clk-ma35d1-divider.c b/drivers/clk/nuvoton/clk-ma35d1-divider.c
new file mode 100644
index 000000000000..340a889a1417
--- /dev/null
+++ b/drivers/clk/nuvoton/clk-ma35d1-divider.c
@@ -0,0 +1,140 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2023 Nuvoton Technology Corp.
+ * Author: Chi-Fang Li <cfli0@xxxxxxxxxxx>
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/regmap.h>
+#include <linux/spinlock.h>
+
+#include "clk-ma35d1.h"
+
+#define clk_div_mask(width)    ((1 << (width)) - 1)
clk-provider.h defines this already, use that?

Yes, I will remove this.

+
+struct ma35d1_adc_clk_div {
+       struct clk_hw hw;
+       void __iomem *reg;
+       u8 shift;
+       u8 width;
+       u32 mask;
+       const struct clk_div_table *table;
+       /* protects concurrent access to clock divider registers */
+       spinlock_t *lock;
+};
+
+static inline struct ma35d1_adc_clk_div *to_ma35d1_adc_clk_div(struct clk_hw *_hw)
+{
+       return container_of(_hw, struct ma35d1_adc_clk_div, hw);
+}
+
+static inline unsigned long ma35d1_clkdiv_recalc_rate(struct clk_hw *hw,
+                                                     unsigned long parent_rate)
+{
+       unsigned int val;
+       struct ma35d1_adc_clk_div *dclk = to_ma35d1_adc_clk_div(hw);
+
+       val = readl_relaxed(dclk->reg) >> dclk->shift;
+       val &= clk_div_mask(dclk->width);
+       val += 1;
+       return divider_recalc_rate(hw, parent_rate, val, dclk->table,
+                                  CLK_DIVIDER_ROUND_CLOSEST, dclk->width);
+}
+
+static inline long ma35d1_clkdiv_round_rate(struct clk_hw *hw,
+                                           unsigned long rate,
+                                           unsigned long *prate)
+{
+       struct ma35d1_adc_clk_div *dclk = to_ma35d1_adc_clk_div(hw);
+
+       return divider_round_rate(hw, rate, prate, dclk->table,
+                                 dclk->width, CLK_DIVIDER_ROUND_CLOSEST);
+}
+
+static inline int ma35d1_clkdiv_set_rate(struct clk_hw *hw,
+                                        unsigned long rate,
+                                        unsigned long parent_rate)
+{
+       int value;
+       unsigned long flags = 0;
+       u32 data;
+       struct ma35d1_adc_clk_div *dclk = to_ma35d1_adc_clk_div(hw);
+
+       value = divider_get_val(rate, parent_rate, dclk->table,
+                               dclk->width, CLK_DIVIDER_ROUND_CLOSEST);
+
+       if (dclk->lock)
+               spin_lock_irqsave(dclk->lock, flags);
Can you just always lock? That makes the conditional locking go away.

OK, I will remove the "if (dclk->lock)" check.


+
+       data = readl_relaxed(dclk->reg);
+       data &= ~(clk_div_mask(dclk->width) << dclk->shift);
+       data |= (value - 1) << dclk->shift;
+       data |= dclk->mask;
+
+       writel_relaxed(data, dclk->reg);
+
+       if (dclk->lock)
+               spin_unlock_irqrestore(dclk->lock, flags);
+
+       return 0;
+}
+
+static const struct clk_ops ma35d1_adc_clkdiv_ops = {
+       .recalc_rate = ma35d1_clkdiv_recalc_rate,
+       .round_rate = ma35d1_clkdiv_round_rate,
+       .set_rate = ma35d1_clkdiv_set_rate,
+};
+
+struct clk_hw *ma35d1_reg_adc_clkdiv(struct device *dev, const char *name,
+                                    const char *parent_name,
+                                    spinlock_t *lock,
+                                    unsigned long flags, void __iomem *reg,
+                                    u8 shift, u8 width, u32 mask_bit)
+{
+       struct ma35d1_adc_clk_div *div;
+       struct clk_init_data init;
+       struct clk_div_table *table;
+       u32 max_div, min_div;
+       struct clk_hw *hw;
+       int ret;
+       int i;
+
+       div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL);
+       if (!div)
+               return ERR_PTR(-ENOMEM);
+
+       max_div = clk_div_mask(width) + 1;
+       min_div = 1;
+
+       table = devm_kcalloc(dev, max_div + 1, sizeof(*table), GFP_KERNEL);
+       if (!table)
+               return ERR_PTR(-ENOMEM);
+
+       for (i = 0; i < max_div; i++) {
+               table[i].val = (min_div + i);
Drop useless parenthesis.

I will fix it.


+               table[i].div = 2 * table[i].val;
+       }
+       table[max_div].val = 0;
+       table[max_div].div = 0;
+
+       init.name = name;
+       init.ops = &ma35d1_adc_clkdiv_ops;
+       init.flags |= flags;
+       init.parent_names = parent_name ? &parent_name : NULL;
+       init.num_parents = parent_name ? 1 : 0;
+
+       div->reg = reg;
+       div->shift = shift;
+       div->width = width;
+       div->mask = mask_bit ? BIT(mask_bit) : 0;
+       div->lock = lock;
+       div->hw.init = &init;
+       div->table = table;
+
+       hw = &div->hw;
+       ret = devm_clk_hw_register(dev, hw);
+       if (ret < 0)
Use if (ret) instead.

I will fix it.


+               return ERR_PTR(ret);
+       return hw;
+}
diff --git a/drivers/clk/nuvoton/clk-ma35d1-pll.c b/drivers/clk/nuvoton/clk-ma35d1-pll.c
new file mode 100644
index 000000000000..b36fbda4fa0a
--- /dev/null
+++ b/drivers/clk/nuvoton/clk-ma35d1-pll.c
@@ -0,0 +1,350 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2023 Nuvoton Technology Corp.
+ * Author: Chi-Fang Li <cfli0@xxxxxxxxxxx>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk-provider.h>
+#include <linux/slab.h>
+#include <linux/regmap.h>
+
+#include "clk-ma35d1.h"
+
+#define REG_SYS_RLKTZNS                0x1a4 /* Register Lock Control */
+
+struct ma35d1_clk_pll {
+       struct clk_hw hw;
+       u8 type;
+       u8 mode;
+       unsigned long rate;
+       void __iomem *ctl0_base;
This needs an include for __iomem define. Just include kernel.h for now.

OK, I will add it.

+       void __iomem *ctl1_base;
+       void __iomem *ctl2_base;
+       struct regmap *regmap;
+};
+
+static inline struct ma35d1_clk_pll *to_ma35d1_clk_pll(struct clk_hw *_hw)
+{
+       return container_of(_hw, struct ma35d1_clk_pll, hw);
Missing include as well.

I will add the
#include <linux/container_of.h>


+}
+
+static void ma35d1_unlock_regs(struct ma35d1_clk_pll *pll)
+{
+       int ret;
+
+       do {
+               regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x59);
+               regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x16);
+               regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x88);
+               regmap_read(pll->regmap, REG_SYS_RLKTZNS, &ret);
+       } while (ret == 0);
Any sort of timeout?

As we have discussed in [PATCH v6 08/12] of this patchset, we will remove the register lock.
So, this function will be removed in the next version.

+}
+
+static void ma35d1_lock_regs(struct ma35d1_clk_pll *pll)
+{
+       regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x0);
+}
+
+static unsigned long ma35d1_calc_smic_pll_freq(u32 pll0_ctl0,
+                                              unsigned long parent_rate)
+{
+       u32 m, n, p, outdiv;
+       u64 pll_freq;
+       u32 clk_div_table[] = { 1, 2, 4, 8 };
const

OK, I will add const to the clk_div_table[].

+
+       if (pll0_ctl0 & SPLL0_CTL0_BP)
+               return parent_rate;
+
+       n = FIELD_GET(SPLL0_CTL0_FBDIV, pll0_ctl0);
+       m = FIELD_GET(SPLL0_CTL0_INDIV, pll0_ctl0);
+       p = FIELD_GET(SPLL0_CTL0_OUTDIV, pll0_ctl0);
+       outdiv = clk_div_table[p];
+       pll_freq = (u64)parent_rate * n;
+       do_div(pll_freq, m * outdiv);
+       return (unsigned long)pll_freq;
Remove useless cast.

I will fix it.

+}
+
+static unsigned long ma35d1_calc_pll_freq(u8 mode, u32 *reg_ctl,
+                                         unsigned long parent_rate)
+{
+       u32 m, n, p;
+       u64 pll_freq, x;
+
+       if (reg_ctl[1] & PLL_CTL1_BP)
+               return parent_rate;
+
+       if (mode == PLL_MODE_INT) {
+               n = FIELD_GET(PLL_CTL0_FBDIV, reg_ctl[0]);
+               m = FIELD_GET(PLL_CTL0_INDIV, reg_ctl[0]);
+               p = FIELD_GET(PLL_CTL1_OUTDIV, reg_ctl[1]);
+               pll_freq = (u64)parent_rate * n;
+               do_div(pll_freq, m * p);
+       } else {
+               n = FIELD_GET(PLL_CTL0_FBDIV, reg_ctl[0]);
+               m = FIELD_GET(PLL_CTL0_INDIV, reg_ctl[0]);
+               p = FIELD_GET(PLL_CTL1_OUTDIV, reg_ctl[1]);
The n, m, p are the same, so pull them out of the if-else to deduplicate.

Ok, I will modify it.


+               x = FIELD_GET(PLL_CTL1_FRAC, reg_ctl[1]);
+               /* 2 decimal places floating to integer (ex. 1.23 to 123) */
+               n = n * 100 + ((x * 100) / FIELD_MAX(PLL_CTL1_FRAC));
Is this mult_frac()?

Not exactly. The number is represented as n.x.
Suppose the number is 123.45, then n is 123 and x is 45.
We have it be multiplied with 100 to be integer 12345, which is n.

In the following
pll_freq = ((u64)parent_rate * n) / 100 / m / p;

We can keep precision of n.

It seems not applicable to mult_frac().


+               pll_freq = ((u64)parent_rate * n) / 100 / m / p;
+       }
Add a newline

+       return (unsigned long)pll_freq;
Remove useless cast.

I will fix them.

+}
+
+static int ma35d1_pll_find_closest(struct ma35d1_clk_pll *pll,
+                                  unsigned long rate,
+                                  unsigned long parent_rate,
+                                  u32 *reg_ctl, unsigned long *freq)
+{
+       int p, m, n;
+       int fbdiv_min, fbdiv_max;
+       unsigned long diff = 0xffffffff;
+
+       *freq = 0;
+       if ((rate < PLL_FCLKO_MIN_FREQ) || (rate > PLL_FCLKO_MAX_FREQ))
+               return -EINVAL;
+
+       if (pll->mode == PLL_MODE_INT) {
+               fbdiv_min = FBDIV_MIN;
+               fbdiv_max = FBDIV_MAX;
+       } else {
+               fbdiv_min = FBDIV_FRAC_MIN;
+               fbdiv_max = FBDIV_FRAC_MAX;
+       }
+
+       for (m = INDIV_MIN; m <= INDIV_MAX; m++) {
+               for (n = fbdiv_min; n <= fbdiv_max; n++) {
+                       for (p = OUTDIV_MIN; p <= OUTDIV_MAX; p++) {
+                               unsigned long tmp, fout;
+                               u64 fclk;
+
+                               tmp = parent_rate / m;
+                               if (tmp < PLL_FREF_M_MIN_FREQ ||
+                                   tmp > PLL_FREF_M_MAX_FREQ)
+                                       continue; /* constrain */
+
+                               fclk = (u64)parent_rate * n / m;
+                               /* for 2 decimal places */
+                               if (pll->mode != PLL_MODE_INT)
+                                       fclk /= 100;
+
+                               if (fclk < PLL_FCLK_MIN_FREQ ||
+                                   fclk > PLL_FCLK_MAX_FREQ)
+                                       continue; /* constrain */
+
+                               fout = (unsigned long)(fclk / p);
+                               if (fout < PLL_FCLKO_MIN_FREQ ||
+                                   fout > PLL_FCLKO_MAX_FREQ)
+                                       continue; /* constrain */
+
+                               if (abs(rate - fout) < diff) {
+                                       reg_ctl[0] = FIELD_PREP(PLL_CTL0_INDIV, m) |
+                                                    FIELD_PREP(PLL_CTL0_FBDIV, n);
+                                       reg_ctl[1] = FIELD_PREP(PLL_CTL1_OUTDIV, p);
+                                       *freq = fout;
+                                       diff = abs(rate - fout);
+                                       if (diff == 0)
+                                               break;
+                               }
+                       }
+               }
+       }
+       if (*freq == 0)
+               return -EINVAL; /* cannot find even one valid setting */
+       return 0;
+}
+
+static int ma35d1_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
+                                  unsigned long parent_rate)
+{
+       struct ma35d1_clk_pll *pll = to_ma35d1_clk_pll(hw);
+       u32 reg_ctl[3] = { 0 };
+       unsigned long pll_freq;
+       int ret;
+
+       if ((parent_rate < PLL_FREF_MIN_FREQ) ||
+           (parent_rate > PLL_FREF_MAX_FREQ))
Remove useless parenthesis.

+               return -EINVAL;
+
+       if ((pll->type == MA35D1_CAPLL) || (pll->type == MA35D1_DDRPLL)) {
Remove useless parenthesis.

I will fix them.

+               pr_warn("Nuvoton MA35D1 CAPLL/DDRPLL is read only.\n");
+               return -EACCES;
+       }
+
+       ret = ma35d1_pll_find_closest(pll, rate, parent_rate, reg_ctl, &pll_freq);
+       if (ret != 0)
+               return ret;
+       pll->rate = pll_freq;
+
+       switch (pll->mode) {
+       case PLL_MODE_INT:
+               reg_ctl[0] |= FIELD_PREP(PLL_CTL0_MODE, PLL_MODE_INT);
+               break;
+       case PLL_MODE_FRAC:
+               reg_ctl[0] |= FIELD_PREP(PLL_CTL0_MODE, PLL_MODE_FRAC);
+               break;
+       case PLL_MODE_SS:
+               reg_ctl[0] |= FIELD_PREP(PLL_CTL0_MODE, PLL_MODE_SS) |
+                             FIELD_PREP(PLL_CTL0_SSRATE, PLL_SS_RATE);
+               reg_ctl[2] = FIELD_PREP(PLL_CTL2_SLOPE, PLL_SLOPE);
+               break;
+       }
+       reg_ctl[1] |= PLL_CTL1_PD;
+
+       ma35d1_unlock_regs(pll);
+       writel_relaxed(reg_ctl[0], pll->ctl0_base);
+       writel_relaxed(reg_ctl[1], pll->ctl1_base);
+       writel_relaxed(reg_ctl[2], pll->ctl2_base);
+       ma35d1_lock_regs(pll);
+       return 0;
+}
+
+static unsigned long ma35d1_clk_pll_recalc_rate(struct clk_hw *hw,
+                                               unsigned long parent_rate)
+{
+       struct ma35d1_clk_pll *pll = to_ma35d1_clk_pll(hw);
+       u32 reg_ctl[3];
+       unsigned long pll_freq;
+
+       if ((parent_rate < PLL_FREF_MIN_FREQ) || (parent_rate > PLL_FREF_MAX_FREQ))
Remove useless parenthesis.

I will fix it.


+               return 0;
+
+       switch (pll->type) {
+       case MA35D1_CAPLL:
+               reg_ctl[0] = readl_relaxed(pll->ctl0_base);
+               pll_freq = ma35d1_calc_smic_pll_freq(reg_ctl[0], parent_rate);
return?

Yes, I will have it return here.

+               break;
+       case MA35D1_DDRPLL:
+       case MA35D1_APLL:
+       case MA35D1_EPLL:
+       case MA35D1_VPLL:
+               reg_ctl[0] = readl_relaxed(pll->ctl0_base);
+               reg_ctl[1] = readl_relaxed(pll->ctl1_base);
+               pll_freq = ma35d1_calc_pll_freq(pll->mode, reg_ctl, parent_rate);
return?

+               break;
+       }
+       return pll_freq;
return 0?

Yes, I will fix them.


+}
+
+static long ma35d1_clk_pll_round_rate(struct clk_hw *hw, unsigned long rate,
+                                     unsigned long *parent_rate)
+{
+       struct ma35d1_clk_pll *pll = to_ma35d1_clk_pll(hw);
+       u32 reg_ctl[3] = { 0 };
+       unsigned long pll_freq;
+       long ret;
+
+       if (*parent_rate < PLL_FREF_MIN_FREQ || *parent_rate > PLL_FREF_MAX_FREQ)
+               return -EINVAL;
+
+       ret = ma35d1_pll_find_closest(pll, rate, *parent_rate, reg_ctl, &pll_freq);
+       if (ret != 0)
+               return ret;
+
+       switch (pll->type) {
+       case MA35D1_CAPLL:
+               reg_ctl[0] = readl_relaxed(pll->ctl0_base);
+               pll_freq = ma35d1_calc_smic_pll_freq(reg_ctl[0], *parent_rate);
return?

+               break;
+       case MA35D1_DDRPLL:
+       case MA35D1_APLL:
+       case MA35D1_EPLL:
+       case MA35D1_VPLL:
+               reg_ctl[0] = readl_relaxed(pll->ctl0_base);
+               reg_ctl[1] = readl_relaxed(pll->ctl1_base);
+               pll_freq = ma35d1_calc_pll_freq(pll->mode, reg_ctl, *parent_rate);
return?

+               break;
+       }
+       return pll_freq;
return 0?

Yes, I will fix them.

+}
+
+static int ma35d1_clk_pll_is_prepared(struct clk_hw *hw)
+{
+       struct ma35d1_clk_pll *pll = to_ma35d1_clk_pll(hw);
+       u32 val = readl_relaxed(pll->ctl1_base);
+
+       return val & PLL_CTL1_PD ? 0 : 1;
+}
+
+static int ma35d1_clk_pll_prepare(struct clk_hw *hw)
+{
+       struct ma35d1_clk_pll *pll = to_ma35d1_clk_pll(hw);
+       u32 val;
+
+       if ((pll->type == MA35D1_CAPLL) || (pll->type == MA35D1_DDRPLL)) {
Drop useless parenthesis.

I will fix it.


+               pr_warn("Nuvoton MA35D1 CAPLL/DDRPLL is read only.\n");
+               return -EACCES;
+       }
+
+       ma35d1_unlock_regs(pll);
+       val = readl_relaxed(pll->ctl1_base);
+       val &= ~PLL_CTL1_PD;
+       writel_relaxed(val, pll->ctl1_base);
+       ma35d1_lock_regs(pll);
+       return 0;
+}
+
+static void ma35d1_clk_pll_unprepare(struct clk_hw *hw)
+{
+       struct ma35d1_clk_pll *pll = to_ma35d1_clk_pll(hw);
+       u32 val;
+
+       if ((pll->type == MA35D1_CAPLL) || (pll->type == MA35D1_DDRPLL)) {
Drop useless parenthesis.

I will fix it.


+               pr_warn("Nuvoton MA35D1 CAPLL/DDRPLL is read only.\n");
These read-only checks should be removed and different clk_ops should be
assigned for the read-only type of clks.

OK, I will fix it.

+       } else {
+               val = readl_relaxed(pll->ctl1_base);
+               val |= PLL_CTL1_PD;
+               writel_relaxed(val, pll->ctl1_base);
+       }
+}
+
+static const struct clk_ops ma35d1_clk_pll_ops = {
+       .is_prepared = ma35d1_clk_pll_is_prepared,
+       .prepare = ma35d1_clk_pll_prepare,
+       .unprepare = ma35d1_clk_pll_unprepare,
+       .set_rate = ma35d1_clk_pll_set_rate,
+       .recalc_rate = ma35d1_clk_pll_recalc_rate,
+       .round_rate = ma35d1_clk_pll_round_rate,
+};
+
+struct clk_hw *ma35d1_reg_clk_pll(enum ma35d1_pll_type type,
Pass a device here.

OK, I will add it.

+                                 u8 u8mode, const char *name,
+                                 const char *parent,
+                                 unsigned long targetFreq,
+                                 void __iomem *base,
+                                 struct regmap *regmap)
+{
+       struct ma35d1_clk_pll *pll;
+       struct clk_hw *hw;
+       struct clk_init_data init;
init = {};

+       int ret;
+
+       pll = kmalloc(sizeof(*pll), GFP_KERNEL);
Just use kzalloc(). This isn't a hot path.

I will use devm_kzalloc().

+       if (!pll)
+               return ERR_PTR(-ENOMEM);
+
+       pll->type = type;
+       pll->mode = u8mode;
+       pll->rate = targetFreq;
+       pll->ctl0_base = base + REG_PLL_CTL0_OFFSET;
+       pll->ctl1_base = base + REG_PLL_CTL1_OFFSET;
+       pll->ctl2_base = base + REG_PLL_CTL2_OFFSET;
+       pll->regmap = regmap;
+
+       init.name = name;
+       init.flags = 0;
+       init.parent_names = &parent;
+       init.num_parents = 1;
+       init.ops = &ma35d1_clk_pll_ops;
+       pll->hw.init = &init;
+       hw = &pll->hw;
+
+       ret = clk_hw_register(NULL, hw);
Use devm_

OK, I will fix it.


+       if (ret) {
+               pr_err("failed to register vsi-pll clock!!!\n");
I'm going to put a dev_err_probe() into clk registration code. Remove
this printk.

OK, I will remove it.

+               kfree(pll);
+               return ERR_PTR(ret);
+       }
+       return hw;
+}
diff --git a/drivers/clk/nuvoton/clk-ma35d1.c b/drivers/clk/nuvoton/clk-ma35d1.c
new file mode 100644
index 000000000000..e4d3ced396a3
--- /dev/null
+++ b/drivers/clk/nuvoton/clk-ma35d1.c
@@ -0,0 +1,963 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2023 Nuvoton Technology Corp.
+ * Author: Chi-Fang Li <cfli0@xxxxxxxxxxx>
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <dt-bindings/clock/nuvoton,ma35d1-clk.h>
+
+#include "clk-ma35d1.h"
+
+static DEFINE_SPINLOCK(ma35d1_lock);
+
+static const char *const ca35clk_sel_clks[] = {
Use clk_parent_data instead.

I will fix it.


+       "hxt", "capll", "ddrpll", "dummy"
+};
+
[...]
+
+static inline struct clk_hw *ma35d1_clk_fixed(const char *name, int rate)
+{
+       return clk_hw_register_fixed_rate(NULL, name, NULL, 0, rate);
+}
+
+static inline struct clk_hw *ma35d1_clk_mux(const char *name,
+                                           void __iomem *reg,
+                                           u8 shift, u8 width,
+                                           const char *const *parents,
+                                           int num_parents)
+{
+       return clk_hw_register_mux(NULL, name, parents, num_parents,
+                                  CLK_SET_RATE_NO_REPARENT, reg, shift,
+                                  width, 0, &ma35d1_lock);
+}
+
+static inline struct clk_hw *ma35d1_clk_divider(const char *name,
+                                               const char *parent,
+                                               void __iomem *reg, u8 shift,
+                                               u8 width)
+{
+       return clk_hw_register_divider(NULL, name, parent, CLK_SET_RATE_PARENT,
+                                      reg, shift, width, 0, &ma35d1_lock);
+}
+
+static inline struct clk_hw *ma35d1_clk_divider_pow2(const char *name,
+                                                    const char *parent,
+                                                    void __iomem *reg,
+                                                    u8 shift, u8 width)
+{
+       return clk_hw_register_divider(NULL, name, parent,
+                                      CLK_DIVIDER_POWER_OF_TWO, reg, shift,
+                                      width, 0, &ma35d1_lock);
+}
+
+static inline struct clk_hw *ma35d1_clk_divider_table(const char *name,
+                                       const char *parent,
+                                       void __iomem *reg,
+                                       u8 shift, u8 width,
+                                       const struct clk_div_table *table)
+{
+       return clk_hw_register_divider_table(NULL, name, parent, 0,
+                                            reg, shift, width, 0, table,
+                                            &ma35d1_lock);
+}
+
+static inline struct clk_hw *ma35d1_clk_fixed_factor(const char *name,
+                                                    const char *parent,
+                                                    unsigned int mult,
+                                                    unsigned int div)
+{
+       return clk_hw_register_fixed_factor(NULL, name, parent,
+                                           CLK_SET_RATE_PARENT, mult, div);
+}
+
+static inline struct clk_hw *ma35d1_clk_gate(const char *name,
+                                            const char *parent,
+                                            void __iomem *reg, u8 shift)
+{
+       return clk_hw_register_gate(NULL, name, parent, CLK_SET_RATE_PARENT,
+                                   reg, shift, 0, &ma35d1_lock);
+}
These need to use devm_ to unregister on failure.

OK, I will fix them all.

+
+static int ma35d1_get_pll_setting(struct device_node *clk_node,
+                                 u32 *pllmode, u32 *pllfreq)
+{
+       const char *of_str;
+       int i;
+
+       for (i = 0; i < PLL_MAX_NUM; i++) {
+               if (of_property_read_string_index(clk_node, "nuvoton,pll-mode", i, &of_str))
+                       return -EINVAL;
+               if (!strcmp(of_str, "integer"))
+                       pllmode[i] = PLL_MODE_INT;
+               else if (!strcmp(of_str, "fractional"))
+                       pllmode[i] = PLL_MODE_FRAC;
+               else if (!strcmp(of_str, "spread-spectrum"))
+                       pllmode[i] = PLL_MODE_SS;
+               else
+                       return -EINVAL;
+       }
+       return of_property_read_u32_array(clk_node, "assigned-clock-rates",
The clk framework looks at this property. Why is the driver looking at
it too?

We should not use "assigned-clock-rates", instead we should create a
vendor-specific property to describe the pll frequency array.

I will fix it in the next version.


+                                         pllfreq, PLL_MAX_NUM);
+}
+


Best regards,
Jacky Huang




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux