RE: [PATCH v10 3/4] reset: cix: add sky1 audss auxiliary reset driver

From: Joakim Zhang

Date: Mon Jul 20 2026 - 08:26:31 EST

Hello Philipp,

> -----Original Message-----
> From: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> Sent: Monday, July 20, 2026 3:20 PM
> To: Joakim Zhang <joakim.zhang@xxxxxxxxxxx>; mturquette@xxxxxxxxxxxx;
> sboyd@xxxxxxxxxx; bmasney@xxxxxxxxxx; robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx;
> conor+dt@xxxxxxxxxx
> Cc: cix-kernel-upstream <cix-kernel-upstream@xxxxxxxxxxx>; linux-
> clk@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v10 3/4] reset: cix: add sky1 audss auxiliary reset driver
>
> EXTERNAL EMAIL
>
> CAUTION: Suspicious Email from unusual domain.
>
> On Mo, 2026-07-20 at 13:15 +0800, joakim.zhang@xxxxxxxxxxx wrote:
> > From: Joakim Zhang <joakim.zhang@xxxxxxxxxxx>
> >
> > Add an auxiliary reset controller driver for the AUDSS CRU. Sixteen
> > software reset lines for audio subsystem peripherals are controlled
> > through one register in the CRU register map.
> >
> > The driver is created by the AUDSS clock platform driver and registers
> > the reset controller on the CRU device node.
> >
> > Signed-off-by: Joakim Zhang <joakim.zhang@xxxxxxxxxxx>
> > ---
> > drivers/reset/Kconfig | 13 +++
> > drivers/reset/Makefile | 1 +
> > drivers/reset/reset-sky1-audss.c | 137
> > +++++++++++++++++++++++++++++++
> > 3 files changed, 151 insertions(+)
> > create mode 100644 drivers/reset/reset-sky1-audss.c
> >
> > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig index
> > d009eb0849a3..b19e719f2abe 100644
> > --- a/drivers/reset/Kconfig
> > +++ b/drivers/reset/Kconfig
> > @@ -300,6 +300,19 @@ config RESET_SKY1
> > help
> > This enables the reset controller for Cix Sky1.
> >
> > +config RESET_SKY1_AUDSS
> > + tristate "Cix Sky1 Audio Subsystem reset controller"
> > + depends on ARCH_CIX || COMPILE_TEST
> > + select AUXILIARY_BUS
> > + default CLK_SKY1_AUDSS
> > + help
> > + Support for block-level software reset lines in the Cix Sky1
> > + Audio Subsystem (AUDSS) Clock and Reset Unit. Sixteen reset
> > + outputs for audio peripherals are controlled through the CRU
> > + register map. The driver binds as an auxiliary device from
> > + the AUDSS clock driver. Say M or Y here if you want to build
> > + this driver.
> > +
> > config RESET_SOCFPGA
> > bool "SoCFPGA Reset Driver" if COMPILE_TEST && (!ARM
> || !ARCH_INTEL_SOCFPGA)
> > default ARM && ARCH_INTEL_SOCFPGA diff --git
> > a/drivers/reset/Makefile b/drivers/reset/Makefile index
> > 3e52569bd276..e81407ea3e29 100644
> > --- a/drivers/reset/Makefile
> > +++ b/drivers/reset/Makefile
> > @@ -39,6 +39,7 @@ obj-$(CONFIG_RESET_RZV2H_USB2PHY) +=
> > reset-rzv2h-usb2phy.o
> > obj-$(CONFIG_RESET_SCMI) += reset-scmi.o
> > obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o
> > obj-$(CONFIG_RESET_SKY1) += reset-sky1.o
> > +obj-$(CONFIG_RESET_SKY1_AUDSS) += reset-sky1-audss.o
> > obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o
> > obj-$(CONFIG_RESET_SUNPLUS) += reset-sunplus.o
> > obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o diff --git
> > a/drivers/reset/reset-sky1-audss.c b/drivers/reset/reset-sky1-audss.c
> > new file mode 100644
> > index 000000000000..d31d80e1251a
> > --- /dev/null
> > +++ b/drivers/reset/reset-sky1-audss.c
> > @@ -0,0 +1,137 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Cix Sky1 Audio Subsystem reset controller driver
> > + *
> > + * Copyright 2026 Cix Technology Group Co., Ltd.
> > + */
> > +
> > +#include <dt-bindings/reset/cix,sky1-audss-cru.h>
> > +
> > +#include <linux/auxiliary_bus.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
>
> What is this used for?

Nothing; will drop it.

> > +#include <linux/regmap.h>
> > +#include <linux/reset-controller.h>
> > +
> > +#define SKY1_RESET_SLEEP_MIN_US 50
> > +#define SKY1_RESET_SLEEP_MAX_US 100
>
> Is this delay compatible with all possible modules being reset?
>
> Are min and max from a spec or is this just some safe value?
> Maybe you could only define the _MIN_US and use fsleep() below.

There is no per-block reset timing in the TRM for AUDSS SW_RST; 50 us
is a conservative safe delay. Happy to switch to fsleep(SKY1_RESET_SLEEP_MIN_US)
and drop MAX.

> > +
> > +#define AUDSS_SW_RST 0x78
> > +
> > +struct sky1_audss_reset_map {
> > + unsigned int offset;
> > + unsigned int mask;
> > +};
> > +
> > +struct sky1_audss_reset {
> > + struct reset_controller_dev rcdev;
> > + struct regmap *regmap;
> > + const struct sky1_audss_reset_map *map; };
> > +
> > +static const struct sky1_audss_reset_map sky1_audss_reset_map[] = {
> > + [AUDSS_I2S0_SW_RST] = { AUDSS_SW_RST, BIT(0) },
> > + [AUDSS_I2S1_SW_RST] = { AUDSS_SW_RST, BIT(1) },
> > + [AUDSS_I2S2_SW_RST] = { AUDSS_SW_RST, BIT(2) },
> > + [AUDSS_I2S3_SW_RST] = { AUDSS_SW_RST, BIT(3) },
> > + [AUDSS_I2S4_SW_RST] = { AUDSS_SW_RST, BIT(4) },
> > + [AUDSS_I2S5_SW_RST] = { AUDSS_SW_RST, BIT(5) },
> > + [AUDSS_I2S6_SW_RST] = { AUDSS_SW_RST, BIT(6) },
> > + [AUDSS_I2S7_SW_RST] = { AUDSS_SW_RST, BIT(7) },
> > + [AUDSS_I2S8_SW_RST] = { AUDSS_SW_RST, BIT(8) },
> > + [AUDSS_I2S9_SW_RST] = { AUDSS_SW_RST, BIT(9) },
> > + [AUDSS_WDT_SW_RST] = { AUDSS_SW_RST, BIT(10) },
> > + [AUDSS_TIMER_SW_RST] = { AUDSS_SW_RST, BIT(11) },
> > + [AUDSS_MB0_SW_RST] = { AUDSS_SW_RST, BIT(12) },
> > + [AUDSS_MB1_SW_RST] = { AUDSS_SW_RST, BIT(13) },
> > + [AUDSS_HDA_SW_RST] = { AUDSS_SW_RST, BIT(14) },
> > + [AUDSS_DMAC_SW_RST] = { AUDSS_SW_RST, BIT(15) },
> > +};
> > +
> > +static struct sky1_audss_reset *to_sky1_audss_reset(struct
> > +reset_controller_dev *rcdev) {
> > + return container_of(rcdev, struct sky1_audss_reset, rcdev); }
> > +
> > +static int sky1_audss_reset_set(struct reset_controller_dev *rcdev,
> > + unsigned long id, bool assert) {
> > + struct sky1_audss_reset *priv = to_sky1_audss_reset(rcdev);
> > + const struct sky1_audss_reset_map *signal = &priv->map[id];
> > + unsigned int value = assert ? 0 : signal->mask;
> > +
> > + return regmap_update_bits(priv->regmap, signal->offset,
> > + signal->mask, value);
>
> Consider using regmap_assign_bits() instead.

OK

> > +}
> > +
> > +static int sky1_audss_reset_assert(struct reset_controller_dev *rcdev,
> > + unsigned long id) {
> > + int ret;
> > +
> > + ret = sky1_audss_reset_set(rcdev, id, true);
> > + if (ret)
> > + return ret;
> > +
> > + usleep_range(SKY1_RESET_SLEEP_MIN_US, SKY1_RESET_SLEEP_MAX_US);
> > + return 0;
> > +}
> > +
> > +static int sky1_audss_reset_deassert(struct reset_controller_dev *rcdev,
> > + unsigned long id) {
> > + int ret;
> > +
> > + ret = sky1_audss_reset_set(rcdev, id, false);
> > + if (ret)
> > + return ret;
> > +
> > + usleep_range(SKY1_RESET_SLEEP_MIN_US, SKY1_RESET_SLEEP_MAX_US);
>
> This is the same in both assert and deassert. You could move it into the helper
> function.

Agreed. Will move the delay into sky1_audss_reset_set() and keep
assert/deassert as thin wrappers.

Thanks,
Joakim