Re: [PATCH v3 3/4] pci: pcie_dw_rockchip: drop clk_release_bulk calls

["Heiko Stübner" <heiko@sntech.de>]

Am Mittwoch, 20. Mai 2026, 08:05:29 Mitteleuropäische Sommerzeit schrieb Daniele Briguglio:
> clk_release_bulk() loops clk_disable() over the bulk; the bulk
> array itself is devm_kcalloc()-allocated and freed at device
> detach, so the function does not actually release memory.
> 
> In parse_dt() the clocks are acquired but never enabled, so the
> existing call disables clocks that are off.  In probe() after
> init_port() failure, the inner err_disable_clks: path added in
> the previous patch has already balanced clk_enable_bulk() with
> clk_disable_bulk(), so the outer call is a double-disable.
> 
> With the new gated-fixed-clock driver both cases trigger
> regulator_set_enable_if_allowed(false) on regulators that were
> either never enabled or just disabled, desynchronising the
> regulator enable_count.  On rk3588-rock-5-itx with only one M.2
> slot populated, the failing empty slot would decrement the
> enable_count of the still-needed regulator twice and pull power
> from the populated slot.
> 
> Suggested-by: Jonas Karlman <jonas@kwiboo.se>
> Signed-off-by: Daniele Briguglio <hello@superkali.me>

Acked-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Heiko Stuebner <heiko@sntech.de> # rock-5-itx


Though the whole clk handling is quite strange.
I would assume that it's always a
  - clk-request
      - clk-enable
          something
      - clk-disable
  - clk-release


That a clk-disable also hides in the clk-release is really surprising,
and I guess a lot of places in u-boot will encounter the same
imbalance - just mitigated by the fact that the double-disable
won't matter.

Directly in clk_get_bulk the problem is apparent ...
The function tries to get all the clocks, and on error calls
clk_release_all on the already gotten clocks.

All of these clocks never got enabled, but are getting double-disabled
by the clk_release_all() call.

Very very strange ;-)


Heiko

> ---
>  drivers/pci/pcie_dw_rockchip.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pcie_dw_rockchip.c b/drivers/pci/pcie_dw_rockchip.c
> index 8ea51e642..c1e34df53 100644
> --- a/drivers/pci/pcie_dw_rockchip.c
> +++ b/drivers/pci/pcie_dw_rockchip.c
> @@ -425,7 +425,7 @@ static int rockchip_pcie_parse_dt(struct udevice *dev)
>  rockchip_pcie_parse_dt_err_phy_get_by_index:
>  	/* regulators don't need release */
>  rockchip_pcie_parse_dt_err_supply_regulator:
> -	clk_release_bulk(&priv->clks);
> +	/* clocks don't need release */
>  rockchip_pcie_parse_dt_err_clk_get_bulk:
>  	reset_release_bulk(&priv->rsts);
>  rockchip_pcie_parse_dt_err_reset_get_bulk:
> @@ -476,7 +476,6 @@ static int rockchip_pcie_probe(struct udevice *dev)
>  		return ret;
>  
>  rockchip_pcie_probe_err_init_port:
> -	clk_release_bulk(&priv->clks);
>  	reset_release_bulk(&priv->rsts);
>  	dm_gpio_free(dev, &priv->rst_gpio);
>  
> 
>