RE: [PATCH v10 2/4] clk: cix: add sky1 audss clock controller

[Joakim Zhang]

Hello Brian,

> -----Original Message-----
> From: Brian Masney <bmasney@xxxxxxxxxx>
> Sent: Wednesday, July 22, 2026 6:14 AM
> To: Joakim Zhang <joakim.zhang@xxxxxxxxxxx>
> Cc: mturquette@xxxxxxxxxxxx; sboyd@xxxxxxxxxx; robh@xxxxxxxxxx;
> krzk+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx; p.zabel@xxxxxxxxxxxxxx; cix-kernel-
> upstream <cix-kernel-upstream@xxxxxxxxxxx>; linux-clk@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v10 2/4] clk: cix: add sky1 audss clock controller
> 
> EXTERNAL EMAIL
> 
> Hi Joakim,
> 
> A few questions / issues mostly related to pm runtime usage.
> 
> On Mon, Jul 20, 2026 at 01:15:03PM +0800, joakim.zhang@xxxxxxxxxxx wrote:
> > From: Joakim Zhang <joakim.zhang@xxxxxxxxxxx>
> >
> > Add a platform driver for the Cix Sky1 AUDSS CRU. The driver maps the
> > CRU registers and registers mux, divider and gate clocks for DSP,
> > SRAM, HDA, DMAC, I2S, mailbox, watchdog and timer blocks.
> >
> > Four SoC-level audio reference clocks are enabled as inputs to the
> > internal clock tree. The driver releases the AUDSS NOC reset, enables
> > runtime PM and instantiates the auxiliary reset device.
> >
> > Signed-off-by: Joakim Zhang <joakim.zhang@xxxxxxxxxxx>
> > ---
> >  drivers/clk/Kconfig              |    1 +
> >  drivers/clk/Makefile             |    1 +
> >  drivers/clk/cix/Kconfig          |   16 +
> >  drivers/clk/cix/Makefile         |    3 +
> >  drivers/clk/cix/clk-sky1-audss.c | 1211
> > ++++++++++++++++++++++++++++++
> >  5 files changed, 1232 insertions(+)
> >  create mode 100644 drivers/clk/cix/Kconfig  create mode 100644
> > drivers/clk/cix/Makefile  create mode 100644
> > drivers/clk/cix/clk-sky1-audss.c
> >
> > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index
> > 1717ce75a907..cfcaab39068a 100644
> > --- a/drivers/clk/Kconfig
> > +++ b/drivers/clk/Kconfig
> > @@ -509,6 +509,7 @@ source "drivers/clk/actions/Kconfig"
> >  source "drivers/clk/analogbits/Kconfig"
> >  source "drivers/clk/aspeed/Kconfig"
> >  source "drivers/clk/bcm/Kconfig"
> > +source "drivers/clk/cix/Kconfig"
> >  source "drivers/clk/eswin/Kconfig"
> >  source "drivers/clk/hisilicon/Kconfig"
> >  source "drivers/clk/imgtec/Kconfig"
> > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index
> > cc108a75a900..87c992f0df54 100644
> > --- a/drivers/clk/Makefile
> > +++ b/drivers/clk/Makefile
> > @@ -119,6 +119,7 @@ obj-$(CONFIG_ARCH_ARTPEC)         += axis/
> >  obj-$(CONFIG_ARC_PLAT_AXS10X)                += axs10x/
> >  obj-y                                        += bcm/
> >  obj-$(CONFIG_ARCH_BERLIN)            += berlin/
> > +obj-y                                        += cix/
> >  obj-$(CONFIG_ARCH_DAVINCI)           += davinci/
> >  obj-$(CONFIG_COMMON_CLK_ESWIN)               += eswin/
> >  obj-$(CONFIG_ARCH_HISI)                      += hisilicon/
> > diff --git a/drivers/clk/cix/Kconfig b/drivers/clk/cix/Kconfig new
> > file mode 100644 index 000000000000..c92a9a873893
> > --- /dev/null
> > +++ b/drivers/clk/cix/Kconfig
> > @@ -0,0 +1,16 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Audio subsystem clock support for Cixtech SoC family menu "Clock
> > +support for Cixtech audss"
> 
> s/audss/AUDSS/
> 
> Or how about spelling out Audio Subsystem Clock Driver

Will change the menu title to spell out Audio Subsystem (or use AUDSS).

> [snip]
> 
> > +static const struct clk_ops sky1_audss_clk_mux_ops = {
> > +     .get_parent = sky1_audss_clk_mux_get_parent,
> > +     .set_parent = sky1_audss_clk_mux_set_parent,
> > +     .determine_rate = sky1_audss_clk_mux_determine_rate,
> 
> Does this need an enable/disable for the pm_runtime get/put?

Will comments below.

> > +};
> > +
> > +static inline struct sky1_clk_divider *to_sky1_clk_divider(struct
> > +clk_divider *div) {
> > +     return container_of(div, struct sky1_clk_divider, div); }
> > +
> > +static unsigned long sky1_audss_clk_divider_recalc_rate(struct clk_hw *hw,
> > +                                                     unsigned long
> > +parent_rate) {
> > +     struct clk_divider *divider = to_clk_divider(hw);
> > +     struct sky1_clk_divider *sky1_div = to_sky1_clk_divider(divider);
> > +     unsigned int val;
> > +
> > +     regmap_read(sky1_div->regmap, sky1_div->offset, &val);
> > +     val = val >> divider->shift;
> > +     val &= clk_div_mask(divider->width);
> > +
> > +     return divider_recalc_rate(hw, parent_rate, val, divider->table,
> > +                                divider->flags, divider->width); }
> > +
> > +static int sky1_audss_clk_divider_determine_rate(struct clk_hw *hw,
> > +                                              struct clk_rate_request
> > +*req) {
> > +     struct clk_divider *divider = to_clk_divider(hw);
> > +     struct sky1_clk_divider *sky1_div =
> > +to_sky1_clk_divider(divider);
> > +
> > +     /* if read only, just return current value */
> > +     if (divider->flags & CLK_DIVIDER_READ_ONLY) {
> > +             u32 val;
> > +
> > +             regmap_read(sky1_div->regmap, sky1_div->offset, &val);
> > +             val = val >> divider->shift;
> > +             val &= clk_div_mask(divider->width);
> > +
> > +             return divider_ro_determine_rate(hw, req, divider->table,
> > +                                              divider->width,
> > +                                              divider->flags, val);
> > +     }
> > +
> > +     return divider_determine_rate(hw, req, divider->table, divider->width,
> > +                                   divider->flags); }
> > +
> > +static int sky1_audss_clk_divider_set_rate(struct clk_hw *hw,
> > +                                        unsigned long rate,
> > +                                        unsigned long parent_rate) {
> > +     struct clk_divider *divider = to_clk_divider(hw);
> > +     struct sky1_clk_divider *sky1_div = to_sky1_clk_divider(divider);
> > +     int value;
> > +     unsigned long flags = 0;
> > +     u32 val;
> > +
> > +     value = divider_get_val(rate, parent_rate, divider->table,
> > +                             divider->width, divider->flags);
> > +     if (value < 0)
> > +             return value;
> > +
> > +     if (divider->lock)
> > +             spin_lock_irqsave(divider->lock, flags);
> > +     else
> > +             __acquire(divider->lock);
> > +
> > +     if (divider->flags & CLK_DIVIDER_HIWORD_MASK) {
> > +             val = clk_div_mask(divider->width) << (divider->shift + 16);
> > +     } else {
> > +             regmap_read(sky1_div->regmap, sky1_div->offset, &val);
> > +             val &= ~(clk_div_mask(divider->width) << divider->shift);
> > +     }
> > +     val |= (u32)value << divider->shift;
> > +     regmap_write(sky1_div->regmap, sky1_div->offset, val);
> > +
> > +     if (divider->lock)
> > +             spin_unlock_irqrestore(divider->lock, flags);
> > +     else
> > +             __release(divider->lock);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct clk_ops sky1_audss_clk_divider_ops = {
> > +     .recalc_rate = sky1_audss_clk_divider_recalc_rate,
> > +     .determine_rate = sky1_audss_clk_divider_determine_rate,
> > +     .set_rate = sky1_audss_clk_divider_set_rate,
> 
> Does this need an enable/disable for the pm_runtime get/put?

Will comments below.

> > +};
> > +
> > +static inline struct sky1_clk_gate *to_sky1_clk_gate(struct clk_gate
> > +*gate) {
> > +     return container_of(gate, struct sky1_clk_gate, gate); }
> > +
> > +static void sky1_audss_clk_gate_endisable(struct clk_hw *hw, int
> > +enable) {
> > +     struct clk_gate *gate = to_clk_gate(hw);
> > +     struct sky1_clk_gate *sky1_gate = to_sky1_clk_gate(gate);
> > +     int set = gate->flags & CLK_GATE_SET_TO_DISABLE ? 1 : 0;
> > +     unsigned long flags = 0;
> > +     u32 reg;
> > +
> > +     set ^= enable;
> > +
> > +     if (gate->lock)
> > +             spin_lock_irqsave(gate->lock, flags);
> > +     else
> > +             __acquire(gate->lock);
> > +
> > +     if (gate->flags & CLK_GATE_HIWORD_MASK) {
> > +             reg = BIT(gate->bit_idx + 16);
> > +             if (set)
> > +                     reg |= BIT(gate->bit_idx);
> > +     } else {
> > +             regmap_read(sky1_gate->regmap, sky1_gate->offset, &reg);
> > +
> > +             if (set)
> > +                     reg |= BIT(gate->bit_idx);
> > +             else
> > +                     reg &= ~BIT(gate->bit_idx);
> > +     }
> > +
> > +     regmap_write(sky1_gate->regmap, sky1_gate->offset, reg);
> > +
> > +     if (gate->lock)
> > +             spin_unlock_irqrestore(gate->lock, flags);
> > +     else
> > +             __release(gate->lock);
> > +}
> > +
> > +static int sky1_audss_clk_gate_enable(struct clk_hw *hw) {
> > +     sky1_audss_clk_gate_endisable(hw, 1);
> 
> pm_runtime_get ?
> 
> > +
> > +     return 0;
> > +}
> > +
> > +static void sky1_audss_clk_gate_disable(struct clk_hw *hw) {
> > +     sky1_audss_clk_gate_endisable(hw, 0);
> 
> pm_runtime_put ?

No. Runtime PM is handled by CCF: rpm_enabled is set when the provider has PM enabled before registration, and clk_prepare() (as well as set_rate/set_parent) already calls pm_runtime_resume_and_get() on the provider. That runs our runtime_resume before clk ops touch the CRU, so extra get/put in the clk_ops would be redundant. Doing get/put in .enable/.disable would also be wrong because those callbacks can run in atomic context and must not sleep.

> > +}
> > +
> > +static int sky1_audss_clk_gate_is_enabled(struct clk_hw *hw) {
> > +     struct clk_gate *gate = to_clk_gate(hw);
> > +     struct sky1_clk_gate *sky1_gate = to_sky1_clk_gate(gate);
> > +     u32 reg;
> > +
> > +     regmap_read(sky1_gate->regmap, sky1_gate->offset, &reg);
> > +
> > +     /* if a set bit disables this clk, flip it before masking */
> > +     if (gate->flags & CLK_GATE_SET_TO_DISABLE)
> > +             reg ^= BIT(gate->bit_idx);
> > +
> > +     reg &= BIT(gate->bit_idx);
> > +
> > +     return !!reg;
> > +}
> > +
> > +static const struct clk_ops sky1_audss_clk_gate_ops = {
> > +     .enable = sky1_audss_clk_gate_enable,
> > +     .disable = sky1_audss_clk_gate_disable,
> > +     .is_enabled = sky1_audss_clk_gate_is_enabled, };
> > +
> > +static struct clk_hw *sky1_audss_clk_register(struct device *dev,
> > +                                           const char *name,
> > +                                           const char * const *parent_names,
> > +                                           int num_parents,
> > +                                           struct regmap *regmap,
> > +                                           const u32 *mux_table,
> > +                                           struct muxdiv_cfg *mux_cfg,
> > +                                           struct muxdiv_cfg *div_cfg,
> > +                                           struct gate_cfg *gate_cfg,
> > +                                           unsigned long flags,
> > +                                           spinlock_t *lock) {
> > +     const struct clk_ops *sky1_gate_ops = NULL;
> > +     const struct clk_ops *sky1_mux_ops = NULL;
> > +     const struct clk_ops *sky1_div_ops = NULL;
> > +     struct sky1_clk_divider *sky1_div = NULL;
> > +     struct sky1_clk_gate *sky1_gate = NULL;
> > +     struct sky1_clk_mux *sky1_mux = NULL;
> > +     struct clk_hw *hw = ERR_PTR(-ENOMEM);
> > +     struct clk_parent_data *parent_data;
> > +     int i;
> > +
> > +     parent_data = devm_kcalloc(dev, num_parents, sizeof(*parent_data),
> GFP_KERNEL);
> > +     if (!parent_data)
> > +             return ERR_PTR(-ENOMEM);
> > +
> > +     for (i = 0; i < num_parents; i++)
> > +             parent_data[i].name = parent_names[i];
> > +
> > +     if (mux_cfg->offset >= 0) {
> > +             sky1_mux = devm_kzalloc(dev, sizeof(*sky1_mux), GFP_KERNEL);
> > +             if (!sky1_mux)
> > +                     return ERR_PTR(-ENOMEM);
> > +
> > +             sky1_mux->mux.reg = NULL;
> > +             sky1_mux->mux.shift = mux_cfg->shift;
> > +             sky1_mux->mux.mask = BIT(mux_cfg->width) - 1;
> > +             sky1_mux->mux.flags = mux_cfg->flags;
> > +             sky1_mux->mux.table = mux_table;
> > +             sky1_mux->mux.lock = lock;
> > +             sky1_mux_ops = &sky1_audss_clk_mux_ops;
> > +             sky1_mux->regmap = regmap;
> > +             sky1_mux->offset = mux_cfg->offset;
> > +     }
> > +
> > +     if (div_cfg->offset >= 0) {
> > +             sky1_div = devm_kzalloc(dev, sizeof(*sky1_div), GFP_KERNEL);
> > +             if (!sky1_div)
> > +                     return ERR_PTR(-ENOMEM);
> > +
> > +             sky1_div->div.reg = NULL;
> > +             sky1_div->div.shift = div_cfg->shift;
> > +             sky1_div->div.width = div_cfg->width;
> > +             sky1_div->div.flags = div_cfg->flags | CLK_DIVIDER_POWER_OF_TWO;
> > +             sky1_div->div.lock = lock;
> > +             sky1_div_ops = &sky1_audss_clk_divider_ops;
> > +             sky1_div->regmap = regmap;
> > +             sky1_div->offset = div_cfg->offset;
> > +     }
> > +
> > +     if (gate_cfg->offset >= 0) {
> > +             sky1_gate = devm_kzalloc(dev, sizeof(*sky1_gate), GFP_KERNEL);
> > +             if (!sky1_gate)
> > +                     return ERR_PTR(-ENOMEM);
> > +
> > +             sky1_gate->gate.reg = NULL;
> > +             sky1_gate->gate.bit_idx = gate_cfg->shift;
> > +             sky1_gate->gate.flags = gate_cfg->flags;
> > +             sky1_gate->gate.lock = lock;
> > +             sky1_gate_ops = &sky1_audss_clk_gate_ops;
> > +             sky1_gate->regmap = regmap;
> > +             sky1_gate->offset = gate_cfg->offset;
> > +     }
> > +
> > +     hw = devm_clk_hw_register_composite_pdata(dev, name, parent_data,
> num_parents,
> > +                                             sky1_mux ? &sky1_mux->mux.hw : NULL,
> sky1_mux_ops,
> > +                                             sky1_div ? &sky1_div->div.hw : NULL, sky1_div_ops,
> > +                                             sky1_gate ? &sky1_gate->gate.hw : NULL,
> sky1_gate_ops,
> > +                                             flags);
> > +     if (IS_ERR(hw)) {
> > +             dev_err(dev, "register %s clock failed with err = %ld\n",
> > +                     name, PTR_ERR(hw));
> > +             return hw;
> > +     }
> > +
> > +     return hw;
> > +}
> > +
> > +static int sky1_audss_clks_get(struct sky1_audss_clks_priv *priv) {
> > +     const struct sky1_audss_clks_devtype_data *devtype_data = priv-
> >devtype_data;
> > +     int i;
> > +
> > +     for (i = 0; i < devtype_data->clk_num; i++) {
> > +             priv->clks[i] = devm_clk_get(priv->dev, devtype_data->clk_names[i]);
> > +             if (IS_ERR(priv->clks[i]))
> > +                     return dev_err_probe(priv->dev, PTR_ERR(priv->clks[i]),
> > +                                          "failed to get clock %s", devtype_data->clk_names[i]);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int sky1_audss_clks_enable(struct sky1_audss_clks_priv *priv)
> > +{
> > +     const struct sky1_audss_clks_devtype_data *devtype_data = priv-
> >devtype_data;
> > +     int i, err;
> > +
> > +     for (i = 0; i < devtype_data->clk_num; i++) {
> > +             err = clk_prepare_enable(priv->clks[i]);
> > +             if (err) {
> > +                     dev_err(priv->dev, "failed to enable clock %s\n",
> > +                             devtype_data->clk_names[i]);
> > +                     goto err_clks;
> > +             }
> > +     }
> > +
> > +     return 0;
> > +
> > +err_clks:
> > +     while (--i >= 0)
> > +             clk_disable_unprepare(priv->clks[i]);
> > +
> > +     return err;
> > +}
> > +
> > +static void sky1_audss_clks_disable(struct sky1_audss_clks_priv
> > +*priv) {
> > +     const struct sky1_audss_clks_devtype_data *devtype_data = priv-
> >devtype_data;
> > +     int i;
> > +
> > +     for (i = 0; i < devtype_data->clk_num; i++)
> > +             clk_disable_unprepare(priv->clks[i]);
> > +}
> > +
> > +static int sky1_audss_clks_set_rate(struct sky1_audss_clks_priv
> > +*priv) {
> > +     const struct sky1_audss_clks_devtype_data *devtype_data = priv-
> >devtype_data;
> > +     int i, err;
> > +
> > +     for (i = 0; i < devtype_data->clk_num; i++) {
> > +             err = clk_set_rate(priv->clks[i], devtype_data->clk_rate_default[i]);
> > +             if (err) {
> > +                     dev_err(priv->dev, "failed to set clock rate %s\n",
> > +                             devtype_data->clk_names[i]);
> > +                     return err;
> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static void sky1_audss_clk_rpm_cleanup(void *data) {
> > +     struct device *dev = data;
> > +
> > +     if (!pm_runtime_status_suspended(dev))
> > +             pm_runtime_force_suspend(dev);
> > +
> > +     pm_runtime_disable(dev);
> 
> Take a look at pm_runtime_force_suspend() in drivers/base/power/runtime.c. It
> already calls pm_runtime_disable(), so if it's not suspended then there is a double
> disable here.

Good catch. pm_runtime_force_suspend() already calls pm_runtime_disable(). I'll drop the extra disable (or only disable when already suspended).

Thanks,
Joakim