dt-bindings: clock: airoha: Add additional reset for PCIe PERSTOUT

PATCH In review Christian Marangi 3 participants 5 messages Jul 23 – Jul 24

Subsystems (3)

Device tree bindings Common clk Reset controller framework

lore

Share & tools

lore

Christian Marangi

author

2026-07-23 19:01 UTC #0 lore raw

PATCH 1/2 dt-bindings: clock: airoha: Add additional reset for PCIe PERSTOUT

Add additional reset to control PCIe PERSTOUT reset line for each of the 3 PCIe lines.

Signed-off-by Christian Marangi <[email protected]>

Acked-by Krzysztof Kozlowski <[email protected]>

---
 include/dt-bindings/reset/airoha,en7581-reset.h | 4 ++++
 1 file changed, 4 insertions(+)

include/dt-bindings/reset/airoha,en7581-reset.h +4 -0

index 6544a1790b83..25e75534daa9 100644
@@ -62,5 +62,9 @@
 #define EN7581_CPU_TIMER_RST		50
 #define EN7581_PCIE_HB_RST		51
 #define EN7581_XPON_MAC_RST		52
+/* RST_PCIC */
+#define EN7581_PCIC_PERSTOUT0_RST	53
+#define EN7581_PCIC_PERSTOUT1_RST	54
+#define EN7581_PCIC_PERSTOUT2_RST	55
 
 #endif /* __DT_BINDINGS_RESET_CONTROLLER_AIROHA_EN7581_H_ */

Signature · 1 line

2.53.0

Christian Marangi

author

2026-07-23 19:01 UTC #1 lore raw

PATCH 2/2 clk: en7523: add support for dedicated PCIe PERSTOUT reset

Add support for resetting the PCIe lines with the PERSTOUT reset. These special reset are controlled by the PCIC register and are specific to each of the 3 PCIe lines.

Contrary to the dedicated reset register where 0 deassert and 1 assert the reset line for the related bit, for PCIe PERSTOUT reset in the PCIC register, the deassert and assert value is swapped, with 1 for deassert and 0 for assert. (HW have these bit set as 1 by default after a cold boot) This is internally handled in the reset function.

PCI enable/disable are updated to drop PERSTOUT bits in favor dedicated reset handling.

Signed-off-by Christian Marangi <[email protected]>

---
 drivers/clk/clk-en7523.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

drivers/clk/clk-en7523.c +26 ~~-10~~

index 1ab0e2eca5d3..2f53e0b42ead 100644
@@ -338,6 +338,7 @@ static const struct en_clk_desc en7581_base_clks[] = {
 static const u16 en7581_rst_ofs[] = {
 	REG_RST_CTRL2,
 	REG_RST_CTRL1,
+	REG_NP_SCU_PCIC,
 };
 
 static const u16 en751221_rst_ofs[] = {

@@ -450,6 +451,11 @@ static const u16 en7581_rst_map[] = {
 	[EN7581_CPU_TIMER_RST]		= RST_NR_PER_BANK + 28,
 	[EN7581_PCIE_HB_RST]		= RST_NR_PER_BANK + 29,
 	[EN7581_XPON_MAC_RST]		= RST_NR_PER_BANK + 31,
+
+	/* RST_PCIC */
+	[EN7581_PCIC_PERSTOUT0_RST]	= 2 * RST_NR_PER_BANK + 29,
+	[EN7581_PCIC_PERSTOUT1_RST]	= 2 * RST_NR_PER_BANK + 26,
+	[EN7581_PCIC_PERSTOUT2_RST]	= 2 * RST_NR_PER_BANK + 16,
 };
 
 static const u16 en751221_rst_map[] = {

@@ -635,9 +641,7 @@ static int en7581_pci_enable(struct clk_hw *hw)
 	void __iomem *np_base = cg->base;
 	u32 val, mask;
 
-	mask = REG_PCI_CONTROL_REFCLK_EN0 | REG_PCI_CONTROL_REFCLK_EN1 |
-	       REG_PCI_CONTROL_PERSTOUT1 | REG_PCI_CONTROL_PERSTOUT2 |
-	       REG_PCI_CONTROL_PERSTOUT;
+	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);

@@ -650,9 +654,7 @@ static void en7581_pci_disable(struct clk_hw *hw)
 	void __iomem *np_base = cg->base;
 	u32 val, mask;
 
-	mask = REG_PCI_CONTROL_REFCLK_EN0 | REG_PCI_CONTROL_REFCLK_EN1 |
-	       REG_PCI_CONTROL_PERSTOUT1 | REG_PCI_CONTROL_PERSTOUT2 |
-	       REG_PCI_CONTROL_PERSTOUT;
+	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);

@@ -754,11 +756,17 @@ static int en7523_reset_update(struct reset_controller_dev *rcdev,
 			       unsigned long id, bool assert)
 {
 	struct en_rst_data *rst_data = container_of(rcdev, struct en_rst_data, rcdev);
-	void __iomem *addr = rst_data->base + rst_data->bank_ofs[id / RST_NR_PER_BANK];
+	u32 offset = rst_data->bank_ofs[id / RST_NR_PER_BANK];
+	void __iomem *addr = rst_data->base + offset;
+	bool inverted = false;
 	u32 val;
 
+	/* For PCIC reset logic is inverted, 0:assert 1:deassert */
+	if (offset == REG_NP_SCU_PCIC)
+		inverted = true;
+
 	val = readl(addr);
-	if (assert)
+	if (assert ^ inverted)
 		val |= BIT(id % RST_NR_PER_BANK);
 	else
 		val &= ~BIT(id % RST_NR_PER_BANK);

@@ -783,9 +791,17 @@ static int en7523_reset_status(struct reset_controller_dev *rcdev,
 			       unsigned long id)
 {
 	struct en_rst_data *rst_data = container_of(rcdev, struct en_rst_data, rcdev);
-	void __iomem *addr = rst_data->base + rst_data->bank_ofs[id / RST_NR_PER_BANK];
+	u32 offset = rst_data->bank_ofs[id / RST_NR_PER_BANK];
+	void __iomem *addr = rst_data->base + offset;
+	bool inverted = false;
+	u32 val;
+
+	/* For PCIC reset logic is inverted, 0:assert 1:deassert */
+	if (offset == REG_NP_SCU_PCIC)
+		inverted = true;
 
-	return !!(readl(addr) & BIT(id % RST_NR_PER_BANK));
+	val = readl(addr) & BIT(id % RST_NR_PER_BANK);
+	return inverted ? !val : !!val;
 }
 
 static int en7523_reset_xlate(struct reset_controller_dev *rcdev,

Signature · 1 line

2.53.0

[email protected]

AI review

2026-07-23 19:15 UTC #2 lore raw

Re: [PATCH 2/2] clk: en7523: add support for dedicated PCIe PERSTOUT reset

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Read-modify-write data race on the shared REG_PCI_CONTROL/REG_NP_SCU_PCIC register (0x88) between the clock and reset frameworks, alongside missing internal locking in the reset framework.

Pre-existing issues:
- [High] The clock disable callback en7581_pci_disable() calls usleep_range() while in atomic context. --

--- Patch [2]: [PATCH 2/2] clk: en7523: add support for dedicated PCIe PERSTOUT reset --- commit c3a062a3f7b8267d37e6c439a55235bdea9e26a6
Author: Christian Marangi <[email protected]>

clk: en7523: add support for dedicated PCIe PERSTOUT reset

Add support for resetting the PCIe lines with the PERSTOUT reset. These special reset are controlled by the PCIC register and are specific to each of the 3 PCIe lines.

PCI enable/disable are updated to drop PERSTOUT bits in favor dedicated reset handling.

Signed-off-by Christian Marangi <[email protected]>

Quoted 4 lines

drivers/clk/clk-en7523.c +0 -0
index 1ab0e2eca5d32..2f53e0b42ead7 100644

[ ... ]

drivers/clk/clk-en7523.c static int en7581_pci_enable(struct clk_hw *hw) · line 646

Quoted 10 lines

drivers/clk/clk-en7523.c +1 -3

@@ -635,9 +641,7 @@ static int en7581_pci_enable(struct clk_hw *hw)
 	void __iomem *np_base = cg->base;
 	u32 val, mask;
 
-	mask = REG_PCI_CONTROL_REFCLK_EN0 | REG_PCI_CONTROL_REFCLK_EN1 |
-	       REG_PCI_CONTROL_PERSTOUT1 | REG_PCI_CONTROL_PERSTOUT2 |
-	       REG_PCI_CONTROL_PERSTOUT;
+	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);

[Severity: High]
Since this patch delegates the PERSTOUT bits to the reset framework via the REG_NP_SCU_PCIC alias for the same register (0x88), is there a risk of a read-modify-write data race here?

The clock framework modifies the REFCLK_EN bits while holding the CCF enable_lock, but the reset framework updates the PERSTOUT bits without acquiring any shared lock. If clk_enable() and reset_control_assert() are called concurrently, could they clobber each other's register writes?

Quoted 7 lines

drivers/clk/clk-en7523.c +0 -0
 	return 0;
}
static void en7581_pci_disable(struct clk_hw *hw) {
drivers/clk/clk-en7523.c +0 -0
 	struct en_clk_gate *cg = container_of(hw, struct en_clk_gate, hw);

drivers/clk/clk-en7523.c static void en7581_pci_disable(struct clk_hw *hw) · line 660

Quoted 10 lines

drivers/clk/clk-en7523.c +1 -3

 	void __iomem *np_base = cg->base;
 	u32 val, mask;
 
-	mask = REG_PCI_CONTROL_REFCLK_EN0 | REG_PCI_CONTROL_REFCLK_EN1 |
-	       REG_PCI_CONTROL_PERSTOUT1 | REG_PCI_CONTROL_PERSTOUT2 |
-	       REG_PCI_CONTROL_PERSTOUT;
+	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 isn't a bug introduced by this patch, but does calling usleep_range() in the .disable callback trigger a scheduling while atomic bug?

In the Common Clock Framework, .disable callbacks run with the global enable_lock spinlock held and hardware interrupts disabled. Operations requiring sleeping delays usually need to use a non-sleeping delay like udelay() or be moved to the .unprepare callback.

[ ... ]

Quoted 20 lines

drivers/clk/clk-en7523.c +8 -2

@@ -754,11 +756,17 @@ static int en7523_reset_update(struct reset_controller_dev *rcdev,
 			       unsigned long id, bool assert)
 {
 	struct en_rst_data *rst_data = container_of(rcdev, struct en_rst_data, rcdev);
-	void __iomem *addr = rst_data->base + rst_data->bank_ofs[id / RST_NR_PER_BANK];
+	u32 offset = rst_data->bank_ofs[id / RST_NR_PER_BANK];
+	void __iomem *addr = rst_data->base + offset;
+	bool inverted = false;
 	u32 val;
 
+	/* For PCIC reset logic is inverted, 0:assert 1:deassert */
+	if (offset == REG_NP_SCU_PCIC)
+		inverted = true;
+
 	val = readl(addr);
-	if (assert)
+	if (assert ^ inverted)
 		val |= BIT(id % RST_NR_PER_BANK);
 	else
 		val &= ~BIT(id % RST_NR_PER_BANK);

Quoted 1 line

writel(val, addr);

[Severity: High]
Could concurrent reset operations race on this register write?

The reset framework performs an unlocked read-modify-write on the shared register (0x88) to toggle PERSTOUT bits. Since this function lacks an internal spinlock, concurrent resets on different lines within the same bank might race, as the reset core does not serialize operations across different lines.

This also races with the clock framework modifications in en7581_pci_enable(), since they share the same underlying register.

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

Brian Masney

Reviewed-by on v1 2/2

2026-07-24 19:06 UTC #3 lore raw

Re: [PATCH 2/2] clk: en7523: add support for dedicated PCIe PERSTOUT reset

In reply to Christian Marangi

Quoted 10 lines · Christian Marangi

Add support for resetting the PCIe lines with the PERSTOUT reset. These special reset are controlled by the PCIC register and are specific to each of the 3 PCIe lines.
Contrary to the dedicated reset register where 0 deassert and 1 assert the reset line for the related bit, for PCIe PERSTOUT reset in the PCIC register, the deassert and assert value is swapped, with 1 for deassert and 0 for assert. (HW have these bit set as 1 by default after a cold boot) This is internally handled in the reset function.

Quoted 4 lines

PCI enable/disable are updated to drop PERSTOUT bits in favor dedicated reset handling.
Signed-off-by Christian Marangi <[email protected]>

Reviewed-by Brian Masney <[email protected]>

[email protected]

AI review

2026-07-23 19:10 UTC #4 lore raw

In reply to Christian Marangi

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Unprotected read-modify-write race condition in en7523_reset_update() --

--- Patch [1]: [PATCH 1/2] dt-bindings: clock: airoha: Add additional reset for PCIe PERSTOUT --- commit 18436d020bf626a2783583bd282cd113ebcf4808
Author: Christian Marangi <[email protected]>

dt-bindings: clock: airoha: Add additional reset for PCIe PERSTOUT

This commit adds three new reset identifiers for PCIe PERSTOUT lines to the Airoha EN7581 reset bindings.

include/dt-bindings/reset/airoha,en7581-reset.h line 68

Quoted 12 lines

include/dt-bindings/reset/airoha,en7581-reset.h +4 -0

index 6544a1790b83b..25e75534daa94 100644
@@ -62,5 +62,9 @@
 #define EN7581_CPU_TIMER_RST		50
 #define EN7581_PCIE_HB_RST		51
 #define EN7581_XPON_MAC_RST		52
+/* RST_PCIC */
+#define EN7581_PCIC_PERSTOUT0_RST	53
+#define EN7581_PCIC_PERSTOUT1_RST	54
+#define EN7581_PCIC_PERSTOUT2_RST	55

[Severity: High]
This is a pre-existing issue, but does adding these new reset lines expose a race condition in the driver's reset handler?

Looking at en7523_reset_update() in drivers/clk/clk-en7523.c, the code performs an unprotected read-modify-write on the shared reset register:

drivers/clk/clk-en7523.c:en7523_reset_update() {
	...
	val = readl(addr);
	if (assert)
		val |= BIT(id % RST_NR_PER_BANK);
	else
		val &= ~BIT(id % RST_NR_PER_BANK);
	writel(val, addr);
	...
}

If two independent consumer drivers call reset_control_assert() or reset_control_deassert() concurrently for different reset IDs that reside in the same 32-bit register bank, can one of the updates be silently dropped?

Since the reset framework delegates hardware-level locking to the provider driver, it seems a spinlock is needed here to prevent concurrent updates from clobbering each other, which might leave a device permanently in reset or prematurely pull it out of reset.

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1