| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z0cAfKdexQugojoJ@ketchup>
Date: Wed, 27 Nov 2024 11:20:28 +0000
From: Haylen Chu <heylenay@....org>
To: Stephen Boyd <sboyd@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Haylen Chu <heylenay@...look.com>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Michael Turquette <mturquette@...libre.com>,
Rob Herring <robh@...nel.org>
Cc: linux-riscv@...ts.infradead.org, linux-clk@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
Inochi Amaoto <inochiama@...look.com>,
Chen Wang <unicornxdotw@...mail.com>,
Jisheng Zhang <jszhang@...nel.org>
Subject: Re: [PATCH v2 3/3] clk: spacemit: Add clock support for Spacemit K1
SoC
Hi Stephen,
Sorry for such a late reply. FYI, I have sent a v3 and applied most of
your recommendation.
On Thu, Sep 19, 2024 at 04:08:32PM -0700, Stephen Boyd wrote:
> Quoting Haylen Chu (2024-09-16 15:23:10)
> > +static const char * const apb_parent_names[] = {
>
> Please don't use strings for parents. Either use struct clk_parent_data
> or clk_hw pointers directly.
>
> > + "pll1_d96_25p6", "pll1_d48_51p2", "pll1_d96_25p6", "pll1_d24_102p4"
> > +};
Thanks for the hint, all parents are described with struct
clk_parent_data in v3.
> > +static int k1_ccu_probe(struct platform_device *pdev)
> > +{
> > + const struct spacemit_ccu_data *data;
> > + struct regmap *base_map, *lock_map;
> > + struct device *dev = &pdev->dev;
> > + struct spacemit_ccu_priv *priv;
> > + struct device_node *parent;
> > + int ret;
> > +
> > + data = of_device_get_match_data(dev);
> > + if (WARN_ON(!data))
> > + return -EINVAL;
> > +
> > + parent = of_get_parent(dev->of_node);
> > + base_map = syscon_node_to_regmap(parent);
> > + of_node_put(parent);
> > +
> > + if (IS_ERR(base_map))
> > + return dev_err_probe(dev, PTR_ERR(base_map),
> > + "failed to get regmap\n");
> > +
> > + if (data->need_pll_lock) {
> > + lock_map = syscon_regmap_lookup_by_phandle(dev->of_node,
> > + "spacemit,mpmu");
> > + if (IS_ERR(lock_map))
> > + return dev_err_probe(dev, PTR_ERR(lock_map),
> > + "failed to get lock regmap\n");
> > + }
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->data = data;
> > + priv->base = base_map;
> > + priv->lock_base = lock_map;
> > + spin_lock_init(&priv->lock);
> > +
> > + ret = spacemit_ccu_register(dev, priv);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "failed to register clocks");
>
> Missing newline on printk
Corrected in v3.
> > diff --git a/drivers/clk/spacemit/ccu_ddn.c b/drivers/clk/spacemit/ccu_ddn.c
> > +static void ccu_ddn_disable(struct clk_hw *hw)
> > +{
> > + struct ccu_ddn *ddn = hw_to_ccu_ddn(hw);
> > + struct ccu_common *common = &ddn->common;
> > + unsigned long flags;
> > +
> > + if (!ddn->gate)
> > + return;
> > +
> > + spin_lock_irqsave(common->lock, flags);
>
> The regmap can have a lock. Can you use that?
Thanks for the hint. This extra lock is dropped in v3. Since all
register operations to shared MMIO regions are performed through
regmap_update_bits(), there cannot be a race.
> > diff --git a/drivers/clk/spacemit/ccu_mix.c b/drivers/clk/spacemit/ccu_mix.c
> > new file mode 100644
> > index 000000000000..750882b6ed93
> > --- /dev/null
> > +++ b/drivers/clk/spacemit/ccu_mix.c
> > +const struct clk_ops spacemit_ccu_mix_ops = {
> > + .disable = ccu_mix_disable,
> > + .enable = ccu_mix_enable,
> > + .is_enabled = ccu_mix_is_enabled,
> > + .get_parent = ccu_mix_get_parent,
> > + .set_parent = ccu_mix_set_parent,
> > + .determine_rate = ccu_mix_determine_rate,
> > + .round_rate = ccu_mix_round_rate,
>
> Only implement determine_rate
Okay, duplicated round_rate is deleted in v3.
>
> > + .recalc_rate = ccu_mix_recalc_rate,
> > + .set_rate = ccu_mix_set_rate,
> > +};
> > +
> > diff --git a/drivers/clk/spacemit/ccu_pll.c b/drivers/clk/spacemit/ccu_pll.c
> > new file mode 100644
> > index 000000000000..1f0ece6abcac
> > --- /dev/null
> > +++ b/drivers/clk/spacemit/ccu_pll.c
> > @@ -0,0 +1,226 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Spacemit clock type pll
> > + *
> > + * Copyright (c) 2024 SpacemiT Technology Co. Ltd
> > + * Copyright (c) 2024 Haylen Chu <heylenay@...look.com>
> > + */
> > +
> > +#include <linux/clk-provider.h>
> > +#include <linux/regmap.h>
> > +
> > +#include "ccu_common.h"
> > +#include "ccu_pll.h"
> > +
> > +#define PLL_MIN_FREQ 600000000
> > +#define PLL_MAX_FREQ 3400000000
> > +#define PLL_DELAY_TIME 3000
> > +
> > +#define pll_read_swcr1(c, v) ccu_read(ctrl, c, v)
> > +#define pll_read_swcr2(c, v) ccu_read(sel, c, v)
> > +#define pll_read_swcr3(c, v) ccu_read(xtc, c, v)
> > +
> > +#define pll_update_swcr1(c, m, v) ccu_update(ctrl, c, m, v)
> > +#define pll_update_swcr2(c, m, v) ccu_update(sel, c, m, v)
> > +#define pll_update_swcr3(c, m, v) ccu_update(xtc, c, m, v)
>
> Please stop wrapping regmap APIs. Just use them directly.
Thanks, I drop the regmap wrappers for each clock type, but keep
ccu_{read,update} in v3 since they save a lot of keystrokes and make
the code easier to read.
>
> > +
> > +#define PLL_SWCR1_REG5_OFF 0
> > +#define PLL_SWCR1_REG5_MASK GENMASK(7, 0)
> > +#define PLL_SWCR1_REG6_OFF 8
> > +#define PLL_SWCR1_REG6_MASK GENMASK(15, 8)
> > +#define PLL_SWCR1_REG7_OFF 16
> > +#define PLL_SWCR1_REG7_MASK GENMASK(23, 16)
> > +#define PLL_SWCR1_REG8_OFF 24
> > +#define PLL_SWCR1_REG8_MASK GENMASK(31, 24)
> > +
> > +#define PLL_SWCR2_DIVn_EN(n) BIT(n + 1)
> > +#define PLL_SWCR2_ATEST_EN BIT(12)
> > +#define PLL_SWCR2_CKTEST_EN BIT(13)
> > +#define PLL_SWCR2_DTEST_EN BIT(14)
> > +
> > +#define PLL_SWCR3_DIV_FRC_OFF 0
> > +#define PLL_SWCR3_DIV_FRC_MASK GENMASK(23, 0)
> > +#define PLL_SWCR3_DIV_INT_OFF 24
> > +#define PLL_SWCR3_DIV_INT_MASK GENMASK(30, 24)
> > +#define PLL_SWCR3_EN BIT(31)
> > +
> > +static int ccu_pll_is_enabled(struct clk_hw *hw)
> > +{
> > + struct ccu_pll *p = hw_to_ccu_pll(hw);
> > + u32 tmp;
> > +
> > + pll_read_swcr3(&p->common, &tmp);
> > +
> > + return tmp & PLL_SWCR3_EN;
> > +}
> > +
> > +/* frequency unit Mhz, return pll vco freq */
> > +static unsigned long __get_vco_freq(struct clk_hw *hw)
> > +{
> > + unsigned int reg5, reg6, reg7, reg8, size, i;
> > + unsigned int div_int, div_frc;
> > + struct ccu_pll_rate_tbl *freq_pll_regs_table;
> > + struct ccu_pll *p = hw_to_ccu_pll(hw);
> > + struct ccu_common *common = &p->common;
> > + u32 tmp;
> > +
> > + pll_read_swcr1(common, &tmp);
> > + reg5 = (tmp & PLL_SWCR1_REG5_MASK) >> PLL_SWCR1_REG5_OFF;
> > + reg6 = (tmp & PLL_SWCR1_REG6_MASK) >> PLL_SWCR1_REG6_OFF;
> > + reg7 = (tmp & PLL_SWCR1_REG7_MASK) >> PLL_SWCR1_REG7_OFF;
> > + reg8 = (tmp & PLL_SWCR1_REG8_MASK) >> PLL_SWCR1_REG8_OFF;
> > +
> > + pll_read_swcr3(common, &tmp);
> > + div_int = (tmp & PLL_SWCR3_DIV_INT_MASK) >> PLL_SWCR3_DIV_INT_OFF;
> > + div_frc = (tmp & PLL_SWCR3_DIV_FRC_MASK) >> PLL_SWCR3_DIV_FRC_OFF;
> > +
> > + freq_pll_regs_table = p->pll.rate_tbl;
> > + size = p->pll.tbl_size;
> > +
> > + for (i = 0; i < size; i++)
> > + if ((freq_pll_regs_table[i].reg5 == reg5) &&
> > + (freq_pll_regs_table[i].reg6 == reg6) &&
> > + (freq_pll_regs_table[i].reg7 == reg7) &&
> > + (freq_pll_regs_table[i].reg8 == reg8) &&
> > + (freq_pll_regs_table[i].div_int == div_int) &&
> > + (freq_pll_regs_table[i].div_frac == div_frc))
> > + return freq_pll_regs_table[i].rate;
> > +
> > + WARN_ON_ONCE(1);
> > +
> > + return 0;
> > +}
> > +
> > +static int ccu_pll_enable(struct clk_hw *hw)
> > +{
> > + struct ccu_pll *p = hw_to_ccu_pll(hw);
> > + struct ccu_common *common = &p->common;
> > + unsigned long flags;
> > + unsigned int tmp;
> > + int ret;
> > +
> > + if (ccu_pll_is_enabled(hw))
> > + return 0;
> > +
> > + spin_lock_irqsave(common->lock, flags);
> > +
> > + pll_update_swcr3(common, PLL_SWCR3_EN, PLL_SWCR3_EN);
> > +
> > + spin_unlock_irqrestore(common->lock, flags);
> > +
> > + /* check lock status */
> > + ret = regmap_read_poll_timeout_atomic(common->lock_base,
> > + p->pll.reg_lock,
> > + tmp,
> > + tmp & p->pll.lock_enable_bit,
> > + 5, PLL_DELAY_TIME);
> > +
> > + return ret;
> > +}
> > +
> > +static void ccu_pll_disable(struct clk_hw *hw)
> > +{
> > + struct ccu_pll *p = hw_to_ccu_pll(hw);
> > + struct ccu_common *common = &p->common;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(p->common.lock, flags);
> > +
> > + pll_update_swcr3(common, PLL_SWCR3_EN, 0);
> > +
> > + spin_unlock_irqrestore(common->lock, flags);
> > +}
> > +
> > +/*
> > + * pll rate change requires sequence:
> > + * clock off -> change rate setting -> clock on
> > + * This function doesn't really change rate, but cache the config
> > + */
> > +static int ccu_pll_set_rate(struct clk_hw *hw, unsigned long rate,
> > + unsigned long parent_rate)
> > +{
> > + struct ccu_pll *p = hw_to_ccu_pll(hw);
> > + struct ccu_common *common = &p->common;
> > + struct ccu_pll_config *params = &p->pll;
> > + struct ccu_pll_rate_tbl *entry;
> > + unsigned long old_rate;
> > + unsigned long flags;
> > + bool found = false;
> > + u32 mask, val;
> > + int i;
> > +
> > + if (ccu_pll_is_enabled(hw)) {
> > + pr_err("%s %s is enabled, ignore the setrate!\n",
> > + __func__, __clk_get_name(hw->clk));
> > + return 0;
> > + }
> > +
> > + old_rate = __get_vco_freq(hw);
> > +
> > + for (i = 0; i < params->tbl_size; i++) {
> > + if (rate == params->rate_tbl[i].rate) {
> > + found = true;
> > + entry = ¶ms->rate_tbl[i];
> > + break;
> > + }
> > + }
> > + WARN_ON_ONCE(!found);
> > +
> > + spin_lock_irqsave(common->lock, flags);
> > +
> > + mask = PLL_SWCR1_REG5_MASK | PLL_SWCR1_REG6_MASK;
> > + mask |= PLL_SWCR1_REG7_MASK | PLL_SWCR1_REG8_MASK;
> > + val |= entry->reg5 << PLL_SWCR1_REG5_OFF;
> > + val |= entry->reg6 << PLL_SWCR1_REG6_OFF;
> > + val |= entry->reg7 << PLL_SWCR1_REG7_OFF;
> > + val |= entry->reg8 << PLL_SWCR1_REG8_OFF;
> > + pll_update_swcr1(common, mask, val);
> > +
> > + mask = PLL_SWCR3_DIV_INT_MASK | PLL_SWCR3_DIV_FRC_MASK;
> > + val = entry->div_int << PLL_SWCR3_DIV_INT_OFF;
> > + val |= entry->div_frac << PLL_SWCR3_DIV_FRC_OFF;
> > + pll_update_swcr3(common, mask, val);
> > +
> > + spin_unlock_irqrestore(common->lock, flags);
> > +
> > + return 0;
> > +}
> > +
> > +static unsigned long ccu_pll_recalc_rate(struct clk_hw *hw,
> > + unsigned long parent_rate)
> > +{
> > + return __get_vco_freq(hw);
> > +}
> > +
> > +static long ccu_pll_round_rate(struct clk_hw *hw, unsigned long rate,
> > + unsigned long *prate)
> > +{
> > + struct ccu_pll *p = hw_to_ccu_pll(hw);
> > + struct ccu_pll_config *params = &p->pll;
> > + unsigned long max_rate = 0;
> > + unsigned int i;
> > +
> > + if (rate > PLL_MAX_FREQ || rate < PLL_MIN_FREQ) {
> > + pr_err("%lu rate out of range!\n", rate);
>
> We should simply clamp the rate here. It doesn't matter what 'rate' is
> when this function is called. The callback is supposed to determine what
> the clk rate will be if a consumer called clk_set_rate() with 'rate'.
> Don't fail that if the rate is requested to be larger than max, just
> tell clk_round_rate() that if you ask for something larger you'll get
> PLL_MAX_FREQ.
Thanks for explaining the convention. I have adapted ccu_pll_round_rate
to follow this behavior in v3.
> > + return -EINVAL;
> > + }
> > +
Thanks again for your review and time.
Best regards,
Haylen Chu
Powered by blists - more mailing lists