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

[sashiko-bot@kernel.org]

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?

>  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