Re: [PATCH v10 5/5] clk: en7523: add support for Airoha AN7583 clock

[Christian Marangi <ansuelsmth@gmail.com>]

On Thu, Nov 05, 2026 at 12:25:20PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] clk: en7523: sleeping in atomic context in en7581_pci_disable()
> - [High] clk: en7523: separate spinlocks fail to serialize shared SCU registers
> - [High] clk: en7523: AN7583 MDIO clocks registered as fixed-rate clocks
> --
> 
> commit 0c38944a68dcbbf6c296bc45b4d9669bdee653fb
> Author: Christian Marangi <ansuelsmth@gmail.com>
> 
> clk: en7523: add support for Airoha AN7583 clock
> 
> This commit adds clock and reset controller support for the Airoha AN7583
> SoC, including its base clock descriptors, MDIO clocks, and reset line
> mappings.
> 
> > diff --git a/drivers/clk/clk-en7523.c b/drivers/clk/clk-en7523.c
> > index 979dd9c20a4f2..c1cc6fbc3c019 100644
> > --- a/drivers/clk/clk-en7523.c
> > +++ b/drivers/clk/clk-en7523.c
> [ ... ]
> > @@ -372,6 +381,138 @@ static const struct en_clk_desc en7581_base_clks[] = {
> >  	}
> >  };
> >  
> > +static const struct en_clk_desc an7583_base_clks[] = {
> [ ... ]
> > +	}, {
> > +		.id = AN7583_CLK_MDIO0,
> > +		.name = "mdio0",
> > +
> > +		.base_reg = REG_CRYPTO_CLKSRC2,
> > +
> > +		.base_value = 25000000,
> > +
> > +		.div_bits = 4,
> > +		.div_shift = 15,
> > +		.div_step = 1,
> > +		.div_offset = 1,
> > +	}, {
> > +		.id = AN7583_CLK_MDIO1,
> > +		.name = "mdio1",
> > +
> > +		.base_reg = REG_CRYPTO_CLKSRC2,
> > +
> > +		.base_value = 25000000,
> > +
> > +		.div_bits = 4,
> > +		.div_shift = 19,
> > +		.div_step = 1,
> > +		.div_offset = 1,
> > +	}
> > +};
> 
> [Severity: High]
> Can AN7583_CLK_MDIO0 and AN7583_CLK_MDIO1 work as intended when registered
> via en7581_register_clocks()?
> 
> In drivers/clk/clk-en7523.c, an7583_clk_hw_init() calls
> en7581_register_clocks(), which reads the initial hardware register value
> at probe time and registers every clock in soc_data->base_clks with
> devm_clk_hw_register_fixed_rate():
> 
>     hw = devm_clk_hw_register_fixed_rate(dev, desc->name, NULL, 0, rate);
> 
> However, in drivers/net/mdio/mdio-airoha.c, airoha_mdio_probe() calls
> clk_set_rate(priv->clk, freq) to divide the 25 MHz default MDIO clock down
> to 2.5 MHz. Since clk_fixed_rate_ops does not implement .set_rate,
> .round_rate, or .determine_rate, clk_set_rate() will return 0 without
> programming the 4-bit divider in REG_CRYPTO_CLKSRC2, leaving the MDIO bus
> clock running at its 25 MHz boot default. Should the MDIO clocks implement
> clockops that support setting the divider rate?
>

This is being handled in a different series that is adding exactly this
feature.
 
> >  static const u16 en7581_rst_ofs[] = {
> [ ... ]
> > @@ -1152,6 +1351,43 @@ static int en7581_clk_hw_init(struct platform_device *pdev,
> >  	return en7581_serdes_phy_register(&pdev->dev);
> >  }
> >  
> > +static int an7583_clk_hw_init(struct platform_device *pdev,
> > +			      struct clk_hw_onecell_data *clk_data)
> > +{
> [ ... ]
> > +	en7581_register_clocks(dev, clk_data, map, base);
> > +
> > +	val = readl(base + REG_RST_CTRL1);
> > +	val |= REG_PCIE_HB_RST;
> > +	writel(val, base + REG_RST_CTRL1);
> > +	val &= ~REG_PCIE_HB_RST;
> > +	writel(val, base + REG_RST_CTRL1);
> > +
> > +	ret = en7581_reset_register(dev, base, an7583_rst_map,
> > +				    ARRAY_SIZE(an7583_rst_map),
> > +				    en7581_rst_ofs);
> > +	if (ret)
> > +		return ret;
> 
> [Severity: High]
> As reported by Sashiko on v6 [1], do cg->lock and rst_data->lock fail to
> serialize concurrent read-modify-write accesses to the shared SCU registers?
> 
> In drivers/clk/clk-en7523.c, an7583_clk_hw_init() registers the PCIe clock
> via en7581_register_clocks() -> en7523_register_pcie_clk() (which allocates
> struct en_clk_gate with its own cg->lock) and registers the reset controller
> via en7581_reset_register() (which allocates struct en_rst_data with a
> separate rst_data->lock) over the same MMIO base.
> 
> In an7583_rst_map, AN7583_PCIC_PERSTOUT0_RST and AN7583_PCIC_PERSTOUT1_RST
> map to bank 2 of en7581_rst_ofs, which is REG_NP_SCU_PCIC (offset 0x88).
> Asserting or deasserting these resets in en7523_reset_update() performs a
> readl()/writel() read-modify-write on base + 0x88 under rst_data->lock:
> 
>     spin_lock_irqsave(&rst_data->lock, flags);
>     val = readl(addr);
>     if (assert ^ inverted)
>             val |= BIT(id % RST_NR_PER_BANK);
>     else
>             val &= ~BIT(id % RST_NR_PER_BANK);
>     writel(val, addr);
>     spin_unlock_irqrestore(&rst_data->lock, flags);
> 
> At the same time, en7581_pci_enable() and en7581_pci_disable() perform a
> readl()/writel() read-modify-write on the same physical register
> REG_PCI_CONTROL (offset 0x88) under cg->lock:
> 
>     spin_lock_irqsave(&cg->lock, flags);
>     val = readl(np_base + REG_PCI_CONTROL);
>     writel(val | mask, np_base + REG_PCI_CONTROL);
>     spin_unlock_irqrestore(&cg->lock, flags);
> 
> Similarly, on EN7523 and EN751221, en7523_pci_prepare() modifies
> REG_RESET_CONTROL1 (offset 0x834) and REG_PCI_CONTROL (offset 0x88) under
> cg->lock, while en7523_reset_update() modifies REG_RST_CTRL1 (offset 0x834)
> under rst_data->lock. In addition, in en7523_pci_prepare(), after releasing
> cg->lock and sleeping in usleep_range(1000, 2000), the code re-acquires
> cg->lock and writes back the cached val without re-reading REG_PCI_CONTROL:
> 
>     spin_lock_irqsave(&cg->lock, flags);
>     val |= REG_PCI_CONTROL_REFCLK_EN1;
>     writel(val, np_base + REG_PCI_CONTROL);
>     spin_unlock_irqrestore(&cg->lock, flags);
> 
> Because cg->lock and rst_data->lock are separate spinlock_t instances, can
> concurrent clock and reset operations race and overwrite each other's bit
> updates in 0x88 and 0x834?
> 
> [ ... ]
> > @@ -1305,6 +1541,18 @@ static const struct en_clk_soc_data en7581_data = {
> >  	.hw_init = en7581_clk_hw_init,
> >  };
> >  
> > +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,
> > +	},
> > +	.hw_init = an7583_clk_hw_init,
> > +};
> 
> [Severity: High]
> As reported by Sashiko on v6 [1], does wiring en7581_pci_disable() to
> .disable in an7583_data cause sleeping in atomic context when the PCIe
> clock is disabled?
> 
> When a consumer calls clk_disable() or clk_disable_unprepare() on
> EN7523_CLK_PCIE, the Common Clock Framework holds enable_lock via
> spin_lock_irqsave() in clk_core_disable_lock() and calls
> core->ops->disable(core->hw).
> 
> In drivers/clk/clk-en7523.c, en7581_pci_disable() calls usleep_range()
> while enable_lock is held with interrupts disabled:
> 
>     spin_lock_irqsave(&cg->lock, flags);
>     val = readl(np_base + REG_PCI_CONTROL);
>     writel(val & ~mask, np_base + REG_PCI_CONTROL);
>     spin_unlock_irqrestore(&cg->lock, flags);
> 
>     usleep_range(1000, 2000);
> 
> Should this sleep be moved to an .unprepare callback (which runs under the
> sleepable prepare_lock mutex, as in en7523_data and en751221_data), or use
> a non-sleeping delay such as udelay()?
> 
> [1] https://lore.kernel.org/r/20260819222743.5044F1F000E9@smtp.kernel.org
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261105121059.11201-1-ansuelsmth@gmail.com?part=5

-- 
	Ansuel