[PATCH 0/2] clk: en7523: two fixes for the EN7581 clock and reset driver

[PATCH 0/2] clk: en7523: two fixes for the EN7581 clock and reset driver

[John Crispin]

Patch 1 stops en7581_pci_disable() from sleeping in a .disable callback,
which the clock core calls with the enable spinlock held.

Patch 2 adds a reset operation, so device_reset() works for consumers
such as i2c-mt7621.

The series applies to next-20261105 and has no dependencies.

John Crispin (2):
  clk: en7523: do not sleep in the EN7581 PCIe clock disable
  clk: en7523: add a reset operation

 drivers/clk/clk-en7523.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)


base-commit: ddfe51b0a0f5dc9e415cc2c8ad8db17532dd426b
-- 
2.34.1

[PATCH 1/2] clk: en7523: do not sleep in the EN7581 PCIe clock disable

[John Crispin]

en7581_pci_disable() is the .disable callback of the PCIe clock gate.
The clock core calls .disable with the enable spinlock held and
interrupts off, but the callback sleeps with usleep_range(). Every boot
where no consumer holds the clock reports "BUG: scheduling while atomic"
from clk_disable_unused(), and so does the unwind of a PCIe probe that
finds no link.

Drop the sleep. Commit 90d4e466c9ea ("PCI: mediatek-gen3: Move reset
delay in mtk_pcie_en7581_power_up()") already moved the wait of the
enable side into the PCIe driver, and nothing depends on a wait after
the disable.

Fixes: 66bc47326ce2 ("clk: en7523: Add EN7581 support")
Signed-off-by: John Crispin <john@phrozen.org>
---
 drivers/clk/clk-en7523.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/clk/clk-en7523.c b/drivers/clk/clk-en7523.c
index 24fdec2e6198..8f63482cbb7b 100644
--- a/drivers/clk/clk-en7523.c
+++ b/drivers/clk/clk-en7523.c
@@ -689,7 +689,6 @@ static void en7581_pci_disable(struct clk_hw *hw)
 	mask = REG_PCI_CONTROL_REFCLK_EN0 | REG_PCI_CONTROL_REFCLK_EN1;
 	val = readl(np_base + REG_PCI_CONTROL);
 	writel(val & ~mask, np_base + REG_PCI_CONTROL);
-	usleep_range(1000, 2000);
 }
 
 static void en7523_register_clocks(struct device *dev, struct clk_hw_onecell_data *clk_data,
-- 
2.34.1

Re: [PATCH 1/2] clk: en7523: do not sleep in the EN7581 PCIe clock disable

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] clk: en7523: data race in en7523_reset_update() [1]
--

commit 1564f332e9a1734f7c62abce0ffb9fd4c355e412
Author: John Crispin <john@phrozen.org>

clk: en7523: do not sleep in the EN7581 PCIe clock disable

This commit removes the usleep_range() call from en7581_pci_disable() to
avoid sleeping in atomic context when the clock core disables the PCIe
clock under the enable spinlock.

> diff --git a/drivers/clk/clk-en7523.c b/drivers/clk/clk-en7523.c
> index 24fdec2e6198c..8f63482cbb7ba 100644
> --- a/drivers/clk/clk-en7523.c
> +++ b/drivers/clk/clk-en7523.c
> @@ -689,7 +689,6 @@ static void en7581_pci_disable(struct clk_hw *hw)
>  	mask = REG_PCI_CONTROL_REFCLK_EN0 | REG_PCI_CONTROL_REFCLK_EN1;
>  	val = readl(np_base + REG_PCI_CONTROL);
>  	writel(val & ~mask, np_base + REG_PCI_CONTROL);
> -	usleep_range(1000, 2000);
>  }

[Severity: High]
This problem wasn't introduced by this patch, but can concurrent accesses
to REG_PCI_CONTROL and the reset bank registers race [1] and lose bit
updates?

en7581_pci_disable(), en7581_pci_enable(), and en7523_reset_update() perform
read-modify-write sequences on shared NP SCU registers (including
REG_PCI_CONTROL on EN7581) without a driver-level spinlock.  Because the
reset controller framework does not serialize .assert and .deassert
callbacks, two callers updating bits in the same register bank, or a PCIe
clock toggle running alongside a reset update on REG_PCI_CONTROL, can
interleave:

CPU0                              CPU1
en7523_reset_update()             en7523_reset_update()
  val = readl(addr);
                                    val = readl(addr);
  val |= BIT(id1);
  writel(val, addr);
                                    val &= ~BIT(id2);
                                    writel(val, addr);  <-- id1 update lost

This overwrites hardware state and can leave peripherals stuck in reset or
corrupt PCIe clock/reset bits.

[1] https://sashiko.dev/#/bug/linux-ef84cdb6-0114-41fc-b76f-33e8bdd47a6f

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111085650.632025-1-john@phrozen.org?part=1

[PATCH 2/2] clk: en7523: add a reset operation

[John Crispin]

The reset controller implements assert, deassert and status but not
reset, so device_reset() fails with -ENOTSUPP. The i2c-mt7621 driver
calls it at probe and after a transfer timeout. Every I2C bus that
references an SCU reset, as on the EN7581 and the AN7589, therefore
logs "I2C reset failed!" and is never reset.

Add a reset operation that asserts and then deasserts the line, as the
MT7621 reset controller does for the same I2C block.

Signed-off-by: John Crispin <john@phrozen.org>
---
 drivers/clk/clk-en7523.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/clk/clk-en7523.c b/drivers/clk/clk-en7523.c
index 8f63482cbb7b..eb893259d470 100644
--- a/drivers/clk/clk-en7523.c
+++ b/drivers/clk/clk-en7523.c
@@ -818,6 +818,18 @@ static int en7523_reset_deassert(struct reset_controller_dev *rcdev,
 	return en7523_reset_update(rcdev, id, false);
 }
 
+static int en7523_reset_reset(struct reset_controller_dev *rcdev,
+			      unsigned long id)
+{
+	int ret;
+
+	ret = en7523_reset_assert(rcdev, id);
+	if (ret)
+		return ret;
+
+	return en7523_reset_deassert(rcdev, id);
+}
+
 static int en7523_reset_status(struct reset_controller_dev *rcdev,
 			       unsigned long id)
 {
@@ -847,6 +859,7 @@ static int en7523_reset_xlate(struct reset_controller_dev *rcdev,
 }
 
 static const struct reset_control_ops en7581_reset_ops = {
+	.reset = en7523_reset_reset,
 	.assert = en7523_reset_assert,
 	.deassert = en7523_reset_deassert,
 	.status = en7523_reset_status,
-- 
2.34.1

Re: [PATCH 2/2] clk: en7523: add a reset operation

[Philipp Zabel]

On Mi, 2026-11-11 at 09:56 +0100, John Crispin wrote:
> The reset controller implements assert, deassert and status but not
> reset, so device_reset() fails with -ENOTSUPP. The i2c-mt7621 driver
> calls it at probe and after a transfer timeout.

Is the i2c-mt7621 driver actually used with any reset controller that
implements .reset but not .assert/.deassert?

If not, it would be better to let i2c-mt7621 use
reset_control_assert/deassert() instead of adding .reset ops to all
reset controller drivers it is used with.

The .reset op is for (compatibility with) self-deasserting resets.

> Every I2C bus that
> references an SCU reset, as on the EN7581 and the AN7589, therefore
> logs "I2C reset failed!" and is never reset.
> 
> Add a reset operation that asserts and then deasserts the line, as the
> MT7621 reset controller does for the same I2C block.
> 
> Signed-off-by: John Crispin <john@phrozen.org>
> ---
>  drivers/clk/clk-en7523.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/clk/clk-en7523.c b/drivers/clk/clk-en7523.c
> index 8f63482cbb7b..eb893259d470 100644
> --- a/drivers/clk/clk-en7523.c
> +++ b/drivers/clk/clk-en7523.c
> @@ -818,6 +818,18 @@ static int en7523_reset_deassert(struct reset_controller_dev *rcdev,
>  	return en7523_reset_update(rcdev, id, false);
>  }
>  
> +static int en7523_reset_reset(struct reset_controller_dev *rcdev,
> +			      unsigned long id)
> +{
> +	int ret;
> +
> +	ret = en7523_reset_assert(rcdev, id);
> +	if (ret)
> +		return ret;

Is having no delay here valid for all possible reset consumers or are
we implicitly assuming that this will only ever be used for specific
i2c resets?

regards
Philipp

Re: [PATCH 2/2] clk: en7523: add a reset operation

[John Crispin]

Hi Philipp,

On Wed, 2026-11-11 at 10:49 +0100, Philipp Zabel wrote:
> Is the i2c-mt7621 driver actually used with any reset controller that
> implements .reset but not .assert/.deassert?

No. Its users are MT7621 and MT7628 (clk-mtmips, which implements
assert, deassert and reset) and EN7581/AN7589 (this driver, assert and
deassert only).

> If not, it would be better to let i2c-mt7621 use
> reset_control_assert/deassert() instead of adding .reset ops to all
> reset controller drivers it is used with.

Agreed. I will drop this patch from v2 and change i2c-mt7621 to assert
and deassert the line instead. That patch goes into v2 of my
"i2c: mt7621: add Airoha AN7589 support" series.

> Is having no delay here valid for all possible reset consumers or are
> we implicitly assuming that this will only ever be used for specific
> i2c resets?

Only for the I2C block, which is a further reason to keep the
sequence in the I2C driver.

Thanks,
John