clk: add support for Airoha AN7583 clock

Document support for Airoha AN7583 clock. This is based on the EN7523 clock schema with the new requirement of the "airoha,chip-scu".

Add additional binding for additional clock and reset lines.

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

---
 .../bindings/clock/airoha,en7523-scu.yaml     | 20 ++++++
 include/dt-bindings/clock/en7523-clk.h        |  3 +
 .../dt-bindings/reset/airoha,an7583-reset.h   | 65 +++++++++++++++++++
 3 files changed, 88 insertions(+)
 create mode 100644 include/dt-bindings/reset/airoha,an7583-reset.h

Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml +1 -0

index eb24a5687639..edecc635807b 100644
@@ -30,6 +30,7 @@ properties:
   compatible:
     items:
       - enum:
+          - airoha,an7583-scu
           - airoha,en7523-scu
           - airoha,en7581-scu
           - econet,en751221-scu

Quoted 26 lines · Christian Marangi

Document support for Airoha AN7583 clock. This is based on the EN7523 clock schema with the new requirement of the "airoha,chip-scu".

Add additional binding for additional clock and reset lines.

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

---
 .../bindings/clock/airoha,en7523-scu.yaml     | 20 ++++++
 include/dt-bindings/clock/en7523-clk.h        |  3 +
 .../dt-bindings/reset/airoha,an7583-reset.h   | 65 +++++++++++++++++++
 3 files changed, 88 insertions(+)
 create mode 100644 include/dt-bindings/reset/airoha,an7583-reset.h

Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml +1 -0

index eb24a5687639..edecc635807b 100644
@@ -30,6 +30,7 @@ properties:
   compatible:
     items:
       - enum:
+          - airoha,an7583-scu
           - airoha,en7523-scu
           - airoha,en7581-scu
           - econet,en751221-scu

Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml properties: · line 57

Quoted 7 lines

Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml +4 -0

@@ -50,12 +51,30 @@ properties:
     description: ID of the controller reset line
     const: 1
 
+  airoha,chip-scu:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: phandle to the Chip SCU providing the registers required
+      for configuring the PCIe related clocks and resets.

This is the clock provider. Clock provider should not be accessing registers of other device to configure its clocks. Either you misrepresented clock hierarchy or devices.

I did search for DTS to try to understand the big pictuer - nothing, no results, no upstream submission to Linux kernel.

Quoted 39 lines · Krzysztof Kozlowski

Quoted 26 lines · Christian Marangi

Document support for Airoha AN7583 clock. This is based on the EN7523 clock schema with the new requirement of the "airoha,chip-scu".

Add additional binding for additional clock and reset lines.

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

---
 .../bindings/clock/airoha,en7523-scu.yaml     | 20 ++++++
 include/dt-bindings/clock/en7523-clk.h        |  3 +
 .../dt-bindings/reset/airoha,an7583-reset.h   | 65 +++++++++++++++++++
 3 files changed, 88 insertions(+)
 create mode 100644 include/dt-bindings/reset/airoha,an7583-reset.h

Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml +1 -0

index eb24a5687639..edecc635807b 100644
@@ -30,6 +30,7 @@ properties:
   compatible:
     items:
       - enum:
+          - airoha,an7583-scu
           - airoha,en7523-scu
           - airoha,en7581-scu
           - econet,en751221-scu

Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml properties: · line 57

Quoted 7 lines

Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml +4 -0

@@ -50,12 +51,30 @@ properties:
     description: ID of the controller reset line
     const: 1
 
+  airoha,chip-scu:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: phandle to the Chip SCU providing the registers required
+      for configuring the PCIe related clocks and resets.

This is the clock provider. Clock provider should not be accessing registers of other device to configure its clocks. Either you misrepresented clock hierarchy or devices.

Quoted 3 lines

I did search for DTS to try to understand the big pictuer - nothing, no results, no upstream submission to Linux kernel.

Hi, you can use en7581 as an example as the implementation is exactly the same register wise.

The register for clock and reset for normal system and PCIe are scattered between the "SCU" and "chip SCU" registers (they are 2 different register block, the name is taken from the programming guide)

- Chip SCU provide register access to some clk gate and clk rate
- SCU provide register for reset, PCIe clock, other clock gate and PHY SERDES.

Upcoming and current airoha clock driver all follow this pattern of declaring one of the 2 register and use a sysconf for the other as the 2 register block are tighlty coupled.

There was a similar phandle in another series adding support for pinctrl for econet EN7528.

This phandle is not present for en7581 just because it's hidden by a direct call to the syscon with syscon_regmap_lookup_by_compatible API.

With AN7583 I'm trying to fix this by making it more explicit.

Does this makes the situation more clear?

Airoha AN7583 SoC will use the same logic used by Airoha EN7581 SoC to register clocks. Generalize it to register clocks defined in soc_data.

Add the clocks definition in EN7581 SoC to support this new implementation.

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

---
 drivers/clk/clk-en7523.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

drivers/clk/clk-en7523.c +0 -0

index 5c45aacb6409..924e4f3e36b6 100644

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

 	const struct clk_ops pcie_ops;
 	int (*hw_init)(struct platform_device *pdev,
 		       struct clk_hw_onecell_data *clk_data);

@@ -714,12 +715,15 @@ static int en7523_clk_hw_init(struct platform_device *pdev,
 static void en7581_register_clocks(struct device *dev, struct clk_hw_onecell_data *clk_data,
 				   struct regmap *map, void __iomem *base)
 {
+	const struct en_clk_soc_data *soc_data;
 	struct clk_hw *hw;
 	u32 rate;
 	int i;
 
-	for (i = 0; i < ARRAY_SIZE(en7581_base_clks); i++) {
-		const struct en_clk_desc *desc = &en7581_base_clks[i];
+	soc_data = device_get_match_data(dev);
+
+	for (i = 0; i < soc_data->num_clocks - 1; i++) {
+		const struct en_clk_desc *desc = &soc_data->base_clks[i];
 		u32 val, reg = desc->div_reg ? desc->div_reg : desc->base_reg;
 		int err;
 

@@ -1014,6 +1018,7 @@ static const struct en_clk_soc_data en7523_data = {
 };
 
 static const struct en_clk_soc_data en7581_data = {
+	.base_clks = en7581_base_clks,
 	/* We increment num_clocks by 1 to account for additional PCIe clock */
 	.num_clocks = ARRAY_SIZE(en7581_base_clks) + 1,
 	.pcie_ops = {

Signature · 1 line

2.53.0

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

New issues:
- [Critical] The macro REG_PCIE_HB_RST is used but never defined, causing a build failure.
- [High] Requesting PCIC resets on AN7583 causes an out-of-bounds read on the en7581_rst_ofs array, leading to undefined hardware behavior or memory corruption.

Pre-existing issues:
- [High] Probe error paths cause a use-after-free by mixing devm_kzalloc with non-devres clk_hw_register() for the PCIe clock.
- [High] Read-modify-write operations on shared registers in the reset and clock controllers lack spinlocks, causing data races and lost writes.
- [High] The clock .disable callback calls usleep_range(), which sleeps while holding an IRQ-disabled spinlock.
--

--- Patch [3]: [PATCH v6 3/3] clk: en7523: add support for Airoha AN7583 clock --- commit e4983d60cb386b35f51bc20a5f0912039bf92780
Author: Christian Marangi <[email protected]>

clk: en7523: add support for Airoha AN7583 clock

Add support for Airoha AN7583 clock and reset.

Airoha AN7583 SoC have the same register address of EN7581 but implement different bits and additional base clocks. Also reset are different with the introduction of 2 dedicated MDIO line and drop of some reset lines.

Quoted 12 lines

drivers/clk/clk-en7523.c +4 -0

index 087ff4568124f..493812c683f53 100644
@@ -505,6 +646,64 @@ static const u16 en751221_rst_map[] = {
 	[EN751221_USB_PHY_P1_RST]	= 3 * RST_NR_PER_BANK + 7,
 };
 
+static const u16 an7583_rst_map[] = {
+	/* RST_CTRL2 */
+	[AN7583_XPON_PHY_RST]		= 0,
+	[AN7583_GPON_OLT_RST]		= 1,

[ ... ]

drivers/clk/clk-en7523.c static const u16 en751221_rst_map[] = { · line 712

Quoted 4 lines

drivers/clk/clk-en7523.c +4 -0

+	/* RST_PCIC */
+	[AN7583_PCIC_PERSTOUT0_RST]	= 2 * RST_NR_PER_BANK + 29,
+	[AN7583_PCIC_PERSTOUT1_RST]	= 2 * RST_NR_PER_BANK + 26,
+};

[Severity: High]
Does this out-of-bounds array access corrupt memory when requesting PCIC resets?

The an7583_rst_map defines these PCIC resets in bank index 2.

When en7581_reset_register() is called later in this patch, it is passed the en7581_rst_ofs array:

	static const u16 en7581_rst_ofs[] = {
		REG_RST_CTRL2,
		REG_RST_CTRL1,
	};

Since en7581_rst_ofs only has 2 elements, when en7523_reset_update() dynamically calculates the bank index via id / RST_NR_PER_BANK, it evaluates to 2. Accessing bank_ofs[2] will read out of bounds.

[ ... ]

Quoted 7 lines

drivers/clk/clk-en7523.c +7 -0

+static int an7583_clk_hw_init(struct platform_device *pdev,
+			      struct clk_hw_onecell_data *clk_data)
+{
+	struct device *dev = &pdev->dev;
+	struct regmap *map;
+	void __iomem *base;
+	u32 val;

drivers/clk/clk-en7523.c static int en7581_clk_hw_init(struct platform_device *pdev, · line 1103

Quoted 10 lines

drivers/clk/clk-en7523.c +10 -0

+
+	map = syscon_regmap_lookup_by_phandle(dev->of_node, "airoha,chip-scu");
+	if (IS_ERR(map))
+		return PTR_ERR(map);
+
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	en7581_register_clocks(dev, clk_data, map, base);

[Severity: High]
This isn't a bug introduced by this patch, but does the probe error path leave us with a use-after-free for the PCIe clock?

In en7523_register_pcie_clk() which is called by en7581_register_clocks(), the clk_hw structure is allocated via devm_kzalloc(), but registered with the non-devres clk_hw_register():

	cg = devm_kzalloc(dev, sizeof(*cg), GFP_KERNEL);
	...
	if (clk_hw_register(dev, &cg->hw))

If a subsequent initialization step fails in en7523_clk_probe() (like hw_init returning an error), the driver core automatically frees the memory via devres, but the clock framework retains dangling pointers to the freed clk_hw.

drivers/clk/clk-en7523.c static int en7581_clk_hw_init(struct platform_device *pdev, · line 1106

Quoted 3 lines

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

+
+	val = readl(base + REG_RST_CTRL1);
+	val |= REG_PCIE_HB_RST;

[Severity: Critical]
Will this break the build due to an undefined macro?

The REG_PCIE_HB_RST macro is not defined anywhere. The existing code seems to use REG_RESET_CONTROL_PCIEHB for this bit instead.

[Severity: High]
This is a pre-existing issue, but could this unlocked read-modify-write race with the reset framework?

en7523_pci_prepare() modifies REG_RESET_CONTROL1 (0x834) without any locks:

	val = readl(np_base + REG_RESET_CONTROL1);
	mask = REG_RESET_CONTROL_PCIE1 | REG_RESET_CONTROL_PCIE2 |
	       REG_RESET_CONTROL_PCIEHB;
	writel(val & ~mask, np_base + REG_RESET_CONTROL1);

Concurrently, en7523_reset_update() performs an unlocked read-modify-write on the same register (REG_RST_CTRL1 is also 0x834):

	val = readl(addr);
	if (assert)
		val |= BIT(id % RST_NR_PER_BANK);
	...
	writel(val, addr);

Since .prepare() runs in process context and resets can be triggered by other probing devices concurrently, this seems likely to cause lost writes.

Quoted 8 lines

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

+	writel(val, base + REG_RST_CTRL1);
+	val &= ~REG_PCIE_HB_RST;
+	writel(val, base + REG_RST_CTRL1);
+
+	return en7581_reset_register(dev, base, an7583_rst_map,
+				     ARRAY_SIZE(an7583_rst_map),
+				     en7581_rst_ofs);
+}

[ ... ]

drivers/clk/clk-en7523.c static const struct en_clk_soc_data en7581_data = { · line 1268

Quoted 9 lines

drivers/clk/clk-en7523.c +9 -0

+static const struct en_clk_soc_data an7583_data = {
+	.base_clks = an7583_base_clks,
+	/* We increment num_clocks by 1 to account for additional PCIe clock */
+	.num_clocks = ARRAY_SIZE(an7583_base_clks) + 1,
+	.pcie_ops = {
+		.is_enabled = en7581_pci_is_enabled,
+		.enable = en7581_pci_enable,
+		.disable = en7581_pci_disable,
+	},

[Severity: High]
This isn't a bug introduced by this patch, but does mapping en7581_pci_disable to the .disable callback result in sleeping while in an atomic context?

The core clock framework invokes .disable callbacks with the enable_lock spinlock held and hardware interrupts disabled.

However, en7581_pci_disable() explicitly yields and sleeps:

	writel(val & ~mask, np_base + REG_PCI_CONTROL);
	usleep_range(1000, 2000);

Could this cause a scheduling while atomic panic when the PCIe clock is disabled?

Quoted 2 lines

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

+	.hw_init = an7583_clk_hw_init,
+};

Signature · 1 line

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