Re: [PATCH v12 3/5] clk: en7523: protect concurrent access to reset and PCI register

[Philipp Zabel <p.zabel@pengutronix.de>]

On Di, 2026-11-10 at 01:07 +0100, Christian Marangi wrote:
> Use spinlock for concurrent access to reset and PCI register to catch case
> when clock is enabled/disabled while a reset is also operated at the same
> time.

Why do reset accesses need to be serialized against clock accesses?

> 
> Some minor rework was needed for PCI unprepare case to permit calling it
> before clk_hw_register and reference the global spinlock.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/clk/clk-en7523.c | 101 ++++++++++++++++++++++++++++++++++++---
>  1 file changed, 95 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/clk/clk-en7523.c b/drivers/clk/clk-en7523.c
> index 2baec734df9f..fbe10e4c3693 100644
> --- a/drivers/clk/clk-en7523.c
> +++ b/drivers/clk/clk-en7523.c
[...]
> @@ -788,21 +866,25 @@ 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);
> +	struct en_clk_priv *priv = dev_get_drvdata(rcdev->dev);
>  	u32 offset = rst_data->bank_ofs[id / RST_NR_PER_BANK];
>  	void __iomem *addr = rst_data->base + offset;
>  	bool inverted = false;
> +	unsigned long flags;
>  	u32 val;
>  
>  	/* For PCIC reset logic is inverted, 0:assert 1:deassert */
>  	if (offset == REG_NP_SCU_PCIC)
>  		inverted = true;
>  
> +	spin_lock_irqsave(&priv->lock, flags);

Consider using guard(spinlock)(&priv->lock) to simplify this.

>  	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(&priv->lock, flags);
>  
>  	return 0;
>  }
> @@ -823,16 +905,21 @@ 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);
> +	struct en_clk_priv *priv = dev_get_drvdata(rcdev->dev);
>  	u32 offset = rst_data->bank_ofs[id / RST_NR_PER_BANK];
>  	void __iomem *addr = rst_data->base + offset;
>  	bool inverted = false;
> +	unsigned long flags;
>  	u32 val;
>  
>  	/* For PCIC reset logic is inverted, 0:assert 1:deassert */
>  	if (offset == REG_NP_SCU_PCIC)
>  		inverted = true;
>  
> +	spin_lock_irqsave(&priv->lock, flags);
>  	val = readl(addr) & BIT(id % RST_NR_PER_BANK);
> +	spin_unlock_irqrestore(&priv->lock, flags);

Why is this read() under a lock? The commit message doesn't tell.

regards
Philipp